Switch the lib to A-labels by default #40
Conversation
This commit bumps the publicsuffix-go dependency to the WIP branch from weppos/publicsuffix-go#40 to support IDN TLDs.
There was a problem hiding this comment.
This is great! I flagged one small typo but otherwise no comments from me.
I've confirmed that Boulder's unit & integration tests pass in letsencrypt/boulder#2339. I also updated our unit tests to explicitly test that we support issuing for an IDN TLD with the updated dependency.
Once this PR lands in master I'll update letsencrypt/boulder#2339 and get the ball rolling on deployment. Thanks again @weppos - really appreciate it!!
| @@ -150,23 +212,23 @@ blogspot.com | |||
| // ===END PRIVATE DOMAINS=== | |||
| ` | |||
|
|
|||
| // FIXME | |||
| p1 := NewRule("blogspot.com") | |||
| // TODO(weppos): abiliuty to set type to a rule. | |||
|
One other minor comment: it would be valuable to update README.md to briefly discuss the IDN support and the default A-label behaviour. |
|
Thanks for the review and confirmation @cpu. Also, ❤️ for the kind words.
That's a fair point. I'll work on it today, and update the branch. Feel free to provide a patch, if you have something specific in mind. One quick question. As you see from d5b7003, I've been starting to provide fixed release as I do for other languages, although in Go this is less relevant. The other main difference is that, while I don't sign each commit, I generally sign each single tag (see https://github.com/weppos/publicsuffix-go/releases/tag/v0.2.0). Would it be something you may want to rely upon? Eg. pull in only fixed release version, instead of linking directly to master? I'm just checking, for the sake of my curiosity. |
I don't have anything specific in mind, just thought it was a worthy feature to discuss. A small note on the default encoding would probably suffice.
I would be in favour of switching to using tagged releases instead of master. I've been slowly cleaning up our dependencies with the goal of moving more towards tagged releases instead of commits. I think it makes reasoning about the dependencies easier overall. |
I'm glad to see we are on the same track. I'll make sure to merge and prep a new release then. |
|
|
||
| Although the PSL list has been traditionally U-label encoded, this library follows the common industry standards and stores the rules in their A-label form. Therefore, unless explicitly mentioned, any method call, comparison or internal representation is expected to be ASCII-compatible encoded (ACE). | ||
|
|
||
| Passing Unicode names to the library may either result is error or unexpected behaviors. |
There was a problem hiding this comment.
"may either result is error or" -> "may either result in error or"
This pull request updates the publicsuffix-go dependency to version 0.3.0, most notably including weppos/publicsuffix-go#40 and support for IDN TLDs. The PA's TestWillingToIssue unit test is updated to confirm that Boulder is WillingToIssue a well formed IDN domain with an IDN TLD. Prior to c5cc328 this causes the PA unit tests to fail as expected with urn:acme:error:malformed :: Name does not end in a public suffix. After c5cc328 everything is 💯 Per CONTRIBUTING.md the unit tests are confirmed to pass: daniel@XXXXXX:~/go/src/github.com/weppos/publicsuffix-go$ git show -s commit 49fe4b0e8276b314e6703300ac26940d9c090a06 Author: Simone Carletti <[email protected]> Date: Mon Nov 21 19:26:37 2016 +0100 Release 0.3.0 daniel@XXXXXX:~/go/src/github.com/weppos/publicsuffix-go$ go test ./... ? github.com/weppos/publicsuffix-go/cmd/gen [no test files] ? github.com/weppos/publicsuffix-go/cmd/load [no test files] ok github.com/weppos/publicsuffix-go/net/publicsuffix 0.007s ok github.com/weppos/publicsuffix-go/publicsuffix 0.042s :heart: :beer: and :tada:'s to @weppos for the upstream work required for this fix. We truly appreciate your volunteer work on the PSL and the publicsuffix-go library. You're the best! This resolves #2277.
Implements the changes described in #31.
The internal representation is now expected to be ASCII. The library will also automatically converts parsed lists from Unicode to ASCII, unless the developers knows the input being ASCII (in this case it's possible to disable the conversion to save processing time).
/cc @jsha @PatF @cpu I used the data you provided as test cases, and I should have covered all the needs. Do you mind to review?
It should be safe to run the test suite for letsencrypt/boulder#2278 around this branch. Once confirmed, I'll merge this branch to master and release.