Acme state fixup #53366
Acme state fixup #53366
Conversation
…w" parameter in "needs_renewal". Pylint-inspired layout fixes.
…ction if nothing/fetch/renew is needed to be done.
|
Does this problem exist in earlier branches (i.e. 2018.3 and 2019.2)? If so, we should point the PR at the earliest branch with the bug. |
…509.read_certificate as alternative method of getting certificate information if tls.cert_info is not available.
…o using x509.read_certificates. Fixed incorrect data placement in textwrap.dedent. Updated test for acme.info to expect dict when using openssl cli.
|
@waynew The behavior with regard to always calling |
|
That makes sense. It would be nice if we could get the fix in the older version(s), but if that's not something that you're able to do, at least fixing it in develop is still 👍 |
|
@waynew How would you suggest I proceed on getting this fix in an older version? Do I have to create a new PR for that, based on the branch to fix, and specifically include the additional utility functions? Or would that better be left for a backport initiated by you? |
|
@github-abcde in the past we've encouraged doing this merge forward kind of thing, but over the past couple of weeks it's become painfully obvious that merge forwards are not a good strategy. The current plan is to encourage rebasing against each branch and creating N pull requests. I've got a script in my fork of Salt, that should help in the process of creating a couple of PRs. Let me know if you have any problems! |
|
@waynew Succeeded, however not with the help of your script, which works under the assumption that all goes well (it didn't, had a rebase conflict when rebasing onto 2018.3). On the bright side, I learned a bit more about git today. |
|
Unfortunately yes, it does assume the happy path 😂 I noticed on the other PR that you had made some maybe more changes? Are those changes that should also apply here? |
|
Looks like a minor docstring problem: |
|
Closing this PR as #55589 has already been merged to master. |
What does this PR do?
Edit: Expanded on the changes in this PR. Clarified the changes to behaviour in test-mode. I earlier stated that test-mode is blatantly ignored, which was incorrect.
It fixes the state
acmeby implementing correct handling of__opts__['test']and also properly detecting if a change is needed or not. It also fixes the returned changes (i.e. no changes returned if no changes were made, and only the changed items returned when there were changes.). Also now returns expected changes in test-mode.Fixed handling of the
window-parameter insalt.modules.acme.needs_renewaland thereby removing the broadExceptioncatching inacme.cert.Moved check if certificate file actually exists into
salt.modules.acme.info, removingPylint now scores both the module and the state, as well as the unittest files with a 10 :)
What issues does this PR fix or reference?
None that I'm aware of.
Previous Behavior
acme.certalways startssalt.modules.acme.certregardless of whether a certificate renewal is needed.No expected changes are reported in testmode, even when this is the case.
acme.certalways returns changes, even when no changes have been made (i.e. when a certificate was not due for renewal and no renewal had been forced).New Behavior
acme.certdoes not startsalt.modules.acme.certwhen the certificate specified does not need renewal and no renewal has been forced. It also now properly returns actual changes instead of always dumping the entire certificate information for both the certificate prior to the call tosalt.modules.acme.certand after.Running
acme.certwith__opts__['test'] == Truereturns changes when a certificate would have been fetched or renewed.Tests written?
Yes
Commits signed with GPG?
Yes