-
-
Notifications
You must be signed in to change notification settings - Fork 631
Clean up dangling OCSP configs #8461
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
Conversation
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 for putting together this PR! The vast majority of it looks good to me, just two main notes:
-
The majority of the line-by-line diffs in //ra/ra.go, //ra/ra_test.go, //issuance/cert_test.go, and //issuance/issuer_test.go are formatting changes unrelated to the semantic content of this PR. Please revert those changes, and include only the diffs related to cleaning up these config items.
-
We won't be able to land this right away. As I've noted in-line, one part of this change will have to be pulled out into a separate PR and landed first. And we still have several of these deprecated config items set in our production configs, so we need to clean those up internally before this can land (tracked in IN-11335).
If you fix this up a bit, I'll make sure the internal config cleanups get prioritized and will let you know when this can land. Thanks again!
P.S. You can use ./t.sh to avoid the problems you ran into with bsetup.
e2fc4ba to
72555e2
Compare
aarongable
left a comment
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 for the updates! LGTM modulo one small necessary change. I've gone ahead and kicked off the CI tests and updated the PR description to be ready for merge once the internal config changes are made.
aarongable
left a comment
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.
LGTM. We'll get a second review and then wait to merge until after IN-11335 is complete.
|
fixed conflict in
|
Fixes #8345
Part of #8177
Warning
Do not merge before IN-11335 is completeHi folks, wanted to try contributing, lemme know if I missed any guidelines.
All I really did was attempt to follow the description from #8345, searched for usages and ran the tests:
docker compose run --use-aliases boulder ./test.shBTW, i was running into a directory/file not found error (iirc it was ipki/minica.pem, during unit tests) which seems to be resolved by
docker compose up bsetup.