-
-
Notifications
You must be signed in to change notification settings - Fork 616
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
SA/ARI: Add method of tracking certificate replacement #7284
Conversation
e241da9
to
2d56a14
Compare
@beautifulentropy, this PR appears to contain configuration changes. Please ensure that a corresponding deployment ticket has been filed with the new configuration values. |
69ff0dd
to
5a880fb
Compare
5a880fb
to
4c20c92
Compare
82e54b9
to
ef4fa18
Compare
ef4fa18
to
07387a3
Compare
You'll need to fix the spelling errors found by |
Converting to draft while I make some changes to the schema. |
c81d83c
to
fdd6ec1
Compare
fdd6ec1
to
35fe14b
Compare
e9bc8d7
to
b39eae5
Compare
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.
Primary logic LGTM. We're missing unit tests for the new functions in model.go. In particular, I think it's important to have tests for
- addReplacementOrder working in both the normal and the already-exists cases; and
- setReplacementOrderFinalized working both when there's a row to update and when there isn't one.
I'm pretty sure that I've got coverage for these cases in the existing SA unit test. I'd happily break that out into the model.go test explicitly though. |
cf0ba0a
to
61c39e7
Compare
61c39e7
to
9049725
Compare
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.
Yeah, I think it'd be much clearer to have individual tests for addReplacementOrder and setReplacementOrderFinalized.
Part of #6732
Part of #7038