-
Notifications
You must be signed in to change notification settings - Fork 90
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
openssl_pkcs12: Add a check for parsed pkcs12 files #145
openssl_pkcs12: Add a check for parsed pkcs12 files #145
Conversation
Signed-off-by: Norman Ziegner <[email protected]>
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.
This is a bit messy, but the whole module is somewhat messy and needs a refactoring. So LGTM :)
Can you add a changelog fragment and add basic tests for this situation?
For tests, it probably makes sense to duplicate this: https://github.com/ansible-collections/community.crypto/blob/main/tests/integration/targets/openssl_pkcs12/tasks/impl.yml#L85-L90 register the result, and make sure in https://github.com/ansible-collections/community.crypto/blob/main/tests/integration/targets/openssl_pkcs12/tests/validate.yml that the task didn't change. Of course more extensive tests are also nice, but such a basic one would already be a big improvement!
Signed-off-by: Norman Ziegner <[email protected]>
And are you sure this is only a problem in check mode, and not also for regular mode? Glancing over the code, I guess both had this issue before (and your fix fixes both). |
Signed-off-by: Norman Ziegner <[email protected]>
Signed-off-by: Norman Ziegner <[email protected]>
Signed-off-by: Norman Ziegner <[email protected]>
Not quite! But 64890dd should now ensure that the correct state is also reported in normal mode (instead of always @felixfontein Could you please review again? |
Signed-off-by: Norman Ziegner <[email protected]>
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
@Normo thanks a lot for fixing this! |
SUMMARY
With this pull request the
parse
action for theopenssl_pkcs12
module is now considered separately in thecheck()
function.Fixes #143
ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION
Without this change, the
parse
action would always result inchanged
state when running in check mode.