Skip to content
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.

Sign up for GitHub

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

Jump to bottom

openssl_certificate: deprecate assertonly provider #60623

Merged
merged 9 commits into from
Aug 18, 2019
Merged

openssl_certificate: deprecate assertonly provider #60623

merged 9 commits into from
Aug 18, 2019

Conversation

felixfontein
Copy link
Contributor

SUMMARY

Deprecates the assertonly provider of openssl_certificate as described in #59972. Fixes #59972.

ISSUE TYPE
  • Bugfix Pull Request
  • Feature Pull Request
COMPONENT NAME

openssl_certificate

@ansibot
Copy link
Contributor

ansibot commented Aug 15, 2019

cc @MarkusTeufelberger @Shaps @Spredzy @Xyon @puiterwijk @resmo
click here for bot help

@ansibot ansibot added affects_2.9 This issue/PR affects Ansible v2.9 bug This issue/PR relates to a bug. community_review In order to be merged, this PR must follow the community review workflow. crypto Crypto community (ACME, openssl, letsencrypt) module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. owner_pr This PR is made by the module's maintainer. support:community This issue/PR relates to code supported by the Ansible community. labels Aug 15, 2019
@ansibot

This comment has been minimized.

Sign in to view
@ansibot
Copy link
Contributor

ansibot commented Aug 15, 2019

cc @MarkusTeufelberger @Shaps @Spredzy @Xyon @puiterwijk @resmo
click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. ci_verified Changes made in this PR are causing tests to fail. labels Aug 15, 2019
@ansibot
Copy link
Contributor

ansibot commented Aug 15, 2019

cc @acozine
click here for bot help

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. support:core This issue/PR relates to code supported by the Ansible Engineering Team. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. owner_pr This PR is made by the module's maintainer. labels Aug 15, 2019
@ansibot
Copy link
Contributor

ansibot commented Aug 15, 2019

cc @acozine
click here for bot help

@felixfontein
Copy link
Contributor Author

While testing this, I found a couple of bugs in the module. I'll push a PR later which fixes them.

MarkusTeufelberger
MarkusTeufelberger reviewed Aug 15, 2019
View reviewed changes
lib/ansible/modules/crypto/openssl_certificate.py
@@ -421,7 +466,98 @@
acme_challenge_path: /etc/ssl/challenges/ansible.com/
force: yes

# The following example shows one assertonly usage using all existing options for
# assertonly, and shows how to emulate the behavior with the openssl_certificate_info,
# openssl_csr_info, openssl_privatekey_info and assert modules:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and then you have a user that wants to ensure mode: u=rx. ;-)

This is tough even with the stat module.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to use assertonly to check this? I'm not sure whether that actually works.

Also, you could use the file module with check_mode: yes to see whether there would be a change, and assert on that :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, the file module in check_mode seems like the way to go then. Thanks for the tip! Maybe we should add it here too (e.g. To check the additional parameters of this module also supported by the file module, you can use the file module in check_mode and see if the file is in the expected state.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertonly does not care about file permissions. I've tested it with devel, stable-2.8, stable-2.7 and stable-2.6. If mode does not match the mode of the file in path, it doesn't complain or change anything.

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Aug 15, 2019
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Aug 15, 2019
@felixfontein
Copy link
Contributor Author

The promised bugfixes are in #60658.

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Aug 15, 2019
@felixfontein felixfontein mentioned this pull request Aug 15, 2019
Closed
@felixfontein
Copy link
Contributor Author

Once #60708 is merged, I'll update this PR to use the _ordered versions for comparison.

@felixfontein felixfontein force-pushed the openssl_certificate-deprecate-assertonly branch 2 times, most recently from 210790c to 59cfa49 Compare August 17, 2019 16:53
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Aug 17, 2019
@felixfontein felixfontein force-pushed the openssl_certificate-deprecate-assertonly branch from 59cfa49 to ecf7af3 Compare August 17, 2019 19:36
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Aug 17, 2019
@MarkusTeufelberger
Copy link
Contributor

Well, then let's

shipit

@felixfontein felixfontein merged commit ceff002 into ansible:devel Aug 18, 2019
@felixfontein felixfontein deleted the openssl_certificate-deprecate-assertonly branch August 18, 2019 03:09
@felixfontein felixfontein mentioned this pull request Aug 25, 2019
Closed
@ansible ansible locked and limited conversation to collaborators Sep 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MarkusTeufelberger MarkusTeufelberger MarkusTeufelberger left review comments

@ffdybuster ffdybuster ffdybuster left review comments

Assignees
No one assigned
Labels
affects_2.9 This issue/PR affects Ansible v2.9 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. crypto Crypto community (ACME, openssl, letsencrypt) docs This issue/PR relates to or includes documentation. has_issue module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

openssl_certificate: deprecate assertonly provider
4 participants
@felixfontein @ansibot @MarkusTeufelberger @ffdybuster