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.

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

Add Support for Updating in iam_server_certificate by Deleting and Creating #2105

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

andrewjroth
Copy link

SUMMARY

This PR relies on #2104 fixing the certificate comparison. Please merge #2104 first.

Previously, changes to the certificate would fail because the module does not support modifications. This change allows for updates by deleting the existing certificate before creating it again. While AWS does not support updating the certificate in-place, it can be supported by delete and replace.

Because this addresses a previously failed scenario, I don't believe this will break any existing usage. The newly created certificate will have the same name and ARN as the previous certificate, meaning it can continue to be referenced the same. The AWS-assigned random ID number will change, but this value is returned in the result from the module so that any downstream usage of the ID will necessarily have a different value and will update accordingly.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

iam_server_certificate

Copy link

Docs Build 📝

Thank you for contribution!✨

The docsite for this PR is available for download as an artifact from this run:
https://github.com/ansible-collections/community.aws/actions/runs/9528161821

You can compare to the docs for the main branch here:
https://ansible-collections.github.io/community.aws/branch/main

File changes:

  • M collections/community/aws/iam_server_certificate_module.html
Click to see the diff comparison.

NOTE: only file modifications are shown here. New and deleted files are excluded.
See the file list and check the published docs to see those files.

diff --git a/home/runner/work/community.aws/community.aws/docsbuild/base/collections/community/aws/iam_server_certificate_module.html b/home/runner/work/community.aws/community.aws/docsbuild/head/collections/community/aws/iam_server_certificate_module.html
index 192fbf1..c039d3f 100644
--- a/home/runner/work/community.aws/community.aws/docsbuild/base/collections/community/aws/iam_server_certificate_module.html
+++ b/home/runner/work/community.aws/community.aws/docsbuild/head/collections/community/aws/iam_server_certificate_module.html
@@ -159,6 +159,7 @@ see <a class="reference internal" href="#ansible-collections-community-aws-iam-s
 <h2><a class="toc-backref" href="#id1" role="doc-backlink">Synopsis</a><a class="headerlink" href="#synopsis" title="Link to this heading"></a></h2>
 <ul class="simple">
 <li><p>Allows for the management of IAM server certificates.</p></li>
+<li><p>If a certificate already exists matching the name, but the certificate or chain is different, the certificate will be deleted and recreated.  This will result in the same <em>ServerCertificateName</em> and <em>Arn</em>, but the <em>ServerCertificateId</em> may be different.</p></li>
 </ul>
 </section>
 <section id="requirements">

Copy link
Contributor

Build failed.
https://ansible.softwarefactory-project.io/zuul/buildset/683905fbd32b47e38fb9671b37680272

✔️ ansible-galaxy-importer SUCCESS in 4m 16s (non-voting)
✔️ build-ansible-collection SUCCESS in 12m 30s
✔️ ansible-test-splitter SUCCESS in 5m 01s
integration-community.aws-1 FAILURE in 6m 27s
Skipped 21 jobs

Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Hi @andrewjroth,

Thanks for taking the time to submit this PR.

  1. Please could you add an integration test (tests/integration/targets/iam_server_certificate)
  2. please add a changelog entry https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs-how-to
  3. This feels like the type of change that could cause pain for anyone relying on "fail" behaviour, especially since the cert ID changes. I think it might be better to add a parameter to "force" the delete/create behaviour, we can default this to false, for now with a deprecation warning that it'll switch to true once the deprecation period has passed (2 years, but as long as the deprecation is there our automation should flag it rather than expecting you to remember to come back.)

@andrewjroth
Copy link
Author

@tremble I commented on my other PR (#2104) about the integration test... I'll also add one here once I get past the errors.

Do you have an example of how to add a parameter with a deprecation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants