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 : Handle setting of permissions while acl is disabled #1168

Merged

Conversation

GomathiselviS
Copy link
Contributor

SUMMARY

As per boto3 aws documentation

When ObjectOwnership is BucketOwnerEnforced - Access control lists (ACLs) are disabled and no longer affect permissions.

Fixes #1137

ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
  • Feature Pull Request
  • New Module Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

@ansibullbot
Copy link

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review module module needs_triage plugins plugin (any type) labels Oct 13, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 3m 50s
✔️ build-ansible-collection SUCCESS in 5m 56s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 9m 53s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 10m 03s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 9m 40s
✔️ ansible-test-units-amazon-aws-python36 SUCCESS in 6m 25s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 6m 30s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 6m 07s
✔️ cloud-tox-py3 SUCCESS in 3m 06s
✔️ ansible-test-splitter SUCCESS in 2m 59s
✔️ integration-amazon.aws-1 SUCCESS in 13m 17s
⚠️ integration-amazon.aws-2 SKIPPED
⚠️ integration-amazon.aws-3 SKIPPED
⚠️ integration-amazon.aws-4 SKIPPED
⚠️ integration-amazon.aws-5 SKIPPED
⚠️ integration-amazon.aws-6 SKIPPED
⚠️ integration-amazon.aws-7 SKIPPED
⚠️ integration-amazon.aws-8 SKIPPED
⚠️ integration-amazon.aws-9 SKIPPED
⚠️ integration-amazon.aws-10 SKIPPED
⚠️ integration-amazon.aws-11 SKIPPED
⚠️ integration-amazon.aws-12 SKIPPED
⚠️ integration-amazon.aws-13 SKIPPED
⚠️ integration-amazon.aws-14 SKIPPED
⚠️ integration-amazon.aws-15 SKIPPED
⚠️ integration-amazon.aws-16 SKIPPED
⚠️ integration-amazon.aws-17 SKIPPED
⚠️ integration-amazon.aws-18 SKIPPED
⚠️ integration-community.aws-1 SKIPPED
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED
⚠️ integration-community.aws-14 SKIPPED
⚠️ integration-community.aws-15 SKIPPED
⚠️ integration-community.aws-16 SKIPPED
⚠️ integration-community.aws-17 SKIPPED
⚠️ integration-community.aws-18 SKIPPED
✔️ ansible-test-changelog SUCCESS in 2m 15s

@tremble
Copy link
Contributor

tremble commented Oct 14, 2022

@GomathiselviS it would be good if you could add an integration test for this.

@ansibullbot ansibullbot added integration tests/integration tests tests labels Oct 14, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

✔️ ansible-galaxy-importer SUCCESS in 4m 00s
✔️ build-ansible-collection SUCCESS in 5m 20s
ansible-test-sanity-aws-ansible-python38 FAILURE in 12m 40s
ansible-test-sanity-aws-ansible-2.12-python38 FAILURE in 9m 24s
ansible-test-sanity-aws-ansible-2.13-python38 FAILURE in 9m 35s
✔️ ansible-test-units-amazon-aws-python36 SUCCESS in 6m 31s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 5m 46s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 6m 08s
✔️ cloud-tox-py3 SUCCESS in 3m 37s
✔️ ansible-test-splitter SUCCESS in 2m 37s
✔️ integration-amazon.aws-1 SUCCESS in 9m 58s
⚠️ integration-amazon.aws-2 SKIPPED
⚠️ integration-amazon.aws-3 SKIPPED
⚠️ integration-amazon.aws-4 SKIPPED
⚠️ integration-amazon.aws-5 SKIPPED
⚠️ integration-amazon.aws-6 SKIPPED
⚠️ integration-amazon.aws-7 SKIPPED
⚠️ integration-amazon.aws-8 SKIPPED
⚠️ integration-amazon.aws-9 SKIPPED
⚠️ integration-amazon.aws-10 SKIPPED
⚠️ integration-amazon.aws-11 SKIPPED
⚠️ integration-amazon.aws-12 SKIPPED
⚠️ integration-amazon.aws-13 SKIPPED
⚠️ integration-amazon.aws-14 SKIPPED
⚠️ integration-amazon.aws-15 SKIPPED
⚠️ integration-amazon.aws-16 SKIPPED
⚠️ integration-amazon.aws-17 SKIPPED
⚠️ integration-amazon.aws-18 SKIPPED
⚠️ integration-community.aws-1 SKIPPED
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED
⚠️ integration-community.aws-14 SKIPPED
⚠️ integration-community.aws-15 SKIPPED
⚠️ integration-community.aws-16 SKIPPED
⚠️ integration-community.aws-17 SKIPPED
⚠️ integration-community.aws-18 SKIPPED
✔️ ansible-test-changelog SUCCESS in 2m 34s

@github-actions
Copy link

github-actions bot commented Oct 17, 2022

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 4m 51s
✔️ build-ansible-collection SUCCESS in 6m 04s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 8m 57s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 9m 42s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 10m 07s
✔️ ansible-test-units-amazon-aws-python36 SUCCESS in 6m 24s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 6m 19s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 6m 13s
✔️ cloud-tox-py3 SUCCESS in 3m 35s
✔️ ansible-test-splitter SUCCESS in 3m 08s
✔️ integration-amazon.aws-1 SUCCESS in 10m 01s
⚠️ integration-amazon.aws-2 SKIPPED
⚠️ integration-amazon.aws-3 SKIPPED
⚠️ integration-amazon.aws-4 SKIPPED
⚠️ integration-amazon.aws-5 SKIPPED
⚠️ integration-amazon.aws-6 SKIPPED
⚠️ integration-amazon.aws-7 SKIPPED
⚠️ integration-amazon.aws-8 SKIPPED
⚠️ integration-amazon.aws-9 SKIPPED
⚠️ integration-amazon.aws-10 SKIPPED
⚠️ integration-amazon.aws-11 SKIPPED
⚠️ integration-amazon.aws-12 SKIPPED
⚠️ integration-amazon.aws-13 SKIPPED
⚠️ integration-amazon.aws-14 SKIPPED
⚠️ integration-amazon.aws-15 SKIPPED
⚠️ integration-amazon.aws-16 SKIPPED
⚠️ integration-amazon.aws-17 SKIPPED
⚠️ integration-amazon.aws-18 SKIPPED
⚠️ integration-community.aws-1 SKIPPED
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED
⚠️ integration-community.aws-14 SKIPPED
⚠️ integration-community.aws-15 SKIPPED
⚠️ integration-community.aws-16 SKIPPED
⚠️ integration-community.aws-17 SKIPPED
⚠️ integration-community.aws-18 SKIPPED
✔️ ansible-test-changelog SUCCESS in 2m 18s

module.params['permission'] = object_acl
if not acl_disabled:
# only use valid object acls for the create_dirkey function
module.params['permission'] = object_acl
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a good idea to modify module.params like this. The variable is supposed to be read-only. It becomes really hard to track the origin of the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@goneri I have not changed the code flow much from what existed. I am not modifying the module.params['permission'], but setting the value based on a condition.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I saw that the existing code is already doing that.

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.

For the sake of getting this bug fixed, +1

I agree with @goneri about the danger of manipulating module.params, @GomathiselviS would you be able to clean that up separately?

@GomathiselviS
Copy link
Contributor Author

For the sake of getting this bug fixed, +1

I agree with @goneri about the danger of manipulating module.params, @GomathiselviS would you be able to clean that up separately?

@tremble @GomathiselviS I will open a separate PR to do the same.

@GomathiselviS GomathiselviS added the mergeit Merge the PR (SoftwareFactory) label Oct 18, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

✔️ ansible-galaxy-importer SUCCESS in 5m 14s
✔️ build-ansible-collection SUCCESS in 4m 51s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 8m 26s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 8m 34s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 10m 33s
✔️ ansible-test-units-amazon-aws-python36 SUCCESS in 6m 04s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 5m 56s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 6m 38s
✔️ cloud-tox-py3 SUCCESS in 3m 04s
✔️ ansible-test-splitter SUCCESS in 2m 51s
✔️ integration-amazon.aws-1 SUCCESS in 9m 36s
⚠️ integration-amazon.aws-2 SKIPPED
⚠️ integration-amazon.aws-3 SKIPPED
⚠️ integration-amazon.aws-4 SKIPPED
⚠️ integration-amazon.aws-5 SKIPPED
⚠️ integration-amazon.aws-6 SKIPPED
⚠️ integration-amazon.aws-7 SKIPPED
⚠️ integration-amazon.aws-8 SKIPPED
⚠️ integration-amazon.aws-9 SKIPPED
⚠️ integration-amazon.aws-10 SKIPPED
⚠️ integration-amazon.aws-11 SKIPPED
⚠️ integration-amazon.aws-12 SKIPPED
⚠️ integration-amazon.aws-13 SKIPPED
⚠️ integration-amazon.aws-14 SKIPPED
⚠️ integration-amazon.aws-15 SKIPPED
⚠️ integration-amazon.aws-16 SKIPPED
⚠️ integration-amazon.aws-17 SKIPPED
⚠️ integration-amazon.aws-18 SKIPPED
⚠️ integration-community.aws-1 SKIPPED
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED
⚠️ integration-community.aws-14 SKIPPED
⚠️ integration-community.aws-15 SKIPPED
⚠️ integration-community.aws-16 SKIPPED
⚠️ integration-community.aws-17 SKIPPED
⚠️ integration-community.aws-18 SKIPPED
✔️ ansible-test-changelog SUCCESS in 2m 20s

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 86b20e9 into ansible-collections:main Oct 18, 2022
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
…collections#1168)

iam_managed_policy - remove unused fail_on_delete parameter

SUMMARY
remove unused fail_on_delete parameter
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
iam_managed_policy
ADDITIONAL INFORMATION
ansible/ansible#63961

Reviewed-by: Markus Bergholz <git@osuv.de>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
…collections#1168)

iam_managed_policy - remove unused fail_on_delete parameter

SUMMARY
remove unused fail_on_delete parameter
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
iam_managed_policy
ADDITIONAL INFORMATION
ansible/ansible#63961

Reviewed-by: Markus Bergholz <git@osuv.de>
mandar242 pushed a commit to mandar242/amazon.aws that referenced this pull request Sep 22, 2023
…collections#1168)

iam_managed_policy - remove unused fail_on_delete parameter

SUMMARY
remove unused fail_on_delete parameter
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
iam_managed_policy
ADDITIONAL INFORMATION
ansible/ansible#63961

Reviewed-by: Markus Bergholz <git@osuv.de>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@7095617
alinabuzachis pushed a commit to alinabuzachis/amazon.aws that referenced this pull request Oct 6, 2023
…collections#1168)

iam_managed_policy - remove unused fail_on_delete parameter

SUMMARY
remove unused fail_on_delete parameter
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
iam_managed_policy
ADDITIONAL INFORMATION
ansible/ansible#63961

Reviewed-by: Markus Bergholz <git@osuv.de>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@7095617
alinabuzachis pushed a commit to alinabuzachis/amazon.aws that referenced this pull request Oct 6, 2023
…collections#1168)

iam_managed_policy - remove unused fail_on_delete parameter

SUMMARY
remove unused fail_on_delete parameter
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
iam_managed_policy
ADDITIONAL INFORMATION
ansible/ansible#63961

Reviewed-by: Markus Bergholz <git@osuv.de>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@7095617
alinabuzachis pushed a commit to alinabuzachis/amazon.aws that referenced this pull request Oct 6, 2023
…collections#1168)

iam_managed_policy - remove unused fail_on_delete parameter

SUMMARY
remove unused fail_on_delete parameter
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
iam_managed_policy
ADDITIONAL INFORMATION
ansible/ansible#63961

Reviewed-by: Markus Bergholz <git@osuv.de>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@7095617
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Oct 24, 2023
…collections#1168)

iam_managed_policy - remove unused fail_on_delete parameter

SUMMARY
remove unused fail_on_delete parameter
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
iam_managed_policy
ADDITIONAL INFORMATION
ansible/ansible#63961

Reviewed-by: Markus Bergholz <git@osuv.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug community_review integration tests/integration mergeit Merge the PR (SoftwareFactory) module module plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws_s3 Folder Creation issue on S3 Buckets with ACLs disabled
5 participants