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

s3_bucket: add check mode support #57112

Open
wants to merge 7 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@zyv
Copy link
Contributor

commented May 29, 2019

SUMMARY

This PR adds check mode support to s3_bucket.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

s3_bucket

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

@zyv, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in #ansible-aws on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@s-hertel

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

Please add tests for check mode.

@goneri goneri requested a review from jillr May 30, 2019

@ansibot ansibot removed the needs_triage label May 30, 2019

@zyv

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

@catcombo, could you please have a go at the integration tests? I believe that it shouldn't be too complicated...

From what I've gathered after having a glance at the linked directory, you just need to go into tasks subdirectory and duplicate mutating blocks so that they come first and feature check_mode: yes - the assertions must still work, however, and subsequent copies that run with check_mode: no should still perform requested changes.

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Code looks good, and I agree having tests would be nice.

From what I've gathered after having a glance at the linked directory, you just need to go into tasks subdirectory and duplicate mutating blocks so that they come first and feature check_mode: yes - the assertions must still work, however, and subsequent copies that run with check_mode: no should still perform requested changes.

Exactly. I'm not sure how the AWS tests currently look like, but the "optimal" pattern is to do every thing four times:

  1. once with check mode - should change
  2. once without check mode - should change
  3. once with check mode - should not change (idempotency)
  4. once without check mode - should not change (idempotency)

(The order of 3 and 4 can of course be swapped.)

If idempotence checks aren't there yet, I would just add 1) and leave 3)+4) for later. (Except if you have time - idempotence checks are always good to have!)

catcombo added some commits Jun 1, 2019

@ansibot ansibot added needs_revision and removed core_review labels Jun 1, 2019

@zyv zyv force-pushed the catcombo:feature/MD-3098-s3-bucket-check-mode branch from daaaf8f to 2b40f15 Jun 2, 2019

@ansibot ansibot added core_review and removed needs_revision labels Jun 2, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

There are a couple of tests which don't have check mode pendants:

  • Create more complex s3_bucket
  • Try to update the same complex s3_bucket
  • Remove a tag for s3_bucket
  • Do not specify any tag to ensure previous tags are not removed
  • Remove all tags
  • Create bucket with dot in name

Is this intentional?

Merge branch 'feature/MD-3098-s3-bucket-check-mode' of github.com:cat…
…combo/ansible into feature/MD-3098-s3-bucket-check-mode
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

@zyv this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.