-
Notifications
You must be signed in to change notification settings - Fork 4
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
[tools/shoestring] fix: renew certs uses generates a new ca cert #1173
base: dev
Are you sure you want to change the base?
Conversation
problem: renew certificates command creates a new ca cert without renew-ca option solution: only generate a new ca cert with the renew-ca option
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1173 +/- ##
==========================================
- Coverage 98.24% 98.22% -0.02%
==========================================
Files 162 157 -5
Lines 6623 6496 -127
Branches 143 143
==========================================
- Hits 6507 6381 -126
+ Misses 116 115 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
👍
# Assert: | ||
assert Path('ca.crt.pem').exists() | ||
assert Path('ca.cnf').exists() | ||
assert ca_certificate_path.exists() |
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 this test is correct because i believe thes would be created by (first) CertificateFactory. to emulate real behavior, i think you want two temporary directories one for each CertificateFactory
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.
hmm..I will take a look but I thought the second CertificateFactory would create another temp folder? 🤔
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.
In which case the factory.reuse_ca_certificate
would fail to create the new_certs
folder since its already present. but will move the second CertificateFactory for clarity
problem: renew certificates command creates a new ca cert without renew-ca option
solution: only generate a new ca cert with the renew-ca option