Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Permit underscores in DNSNames if-and-only-if those certificates are valid for less than 30 days and during BR 1.6.2's permissibility period #660
Permit underscores in DNSNames if-and-only-if those certificates are valid for less than 30 days and during BR 1.6.2's permissibility period #660
Changes from 5 commits
105e598
7bba8a9
1436a00
804fd34
e77ba12
f907368
803e81a
5be5c3c
7e911d3
efc09de
36e1b34
f8758fd
2c68f47
edfbc23
7279c2e
3532b6e
f4376d3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can/should the check from 47 - 49 be moved here?
Might be a way to simplify further. But is a bit of a suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so? If we do this check here then what we would be saying is that this lint does not apply to this certificate at all (which is usually reserved for scenarios such as being an irrelevant cert type or not having a particular field).
I do prefer that method of adding 30 days, though. Far more correct than the naive
time.Hour*24*30
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something something "inclusive", but this predates the 5280 language coming over :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto re:
.test
fixesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (thank you for your patience on this, I've been traveling for business and I almost missed a flight trying to juggle these similiar-but-different certs so I just punted for the week).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT? (e.g. lines 47-62)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I hop back-and-forth between Go/Rust/Python/Java often enough that the naming conventions bleed over each other sometimes.
And yeah, with regard to the certs, they may be duplicates DNS names but I believe that I did generate entirely different ones just for the sake of tests not sharing literally same test certs, which you can easily imagine exploding in someone's face in the future (and also because merge management woulda been a little annoying).