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

adding encryption_bucket_key #866

Closed
wants to merge 2 commits into from
Closed

adding encryption_bucket_key #866

wants to merge 2 commits into from

Conversation

chirag1603
Copy link
Contributor

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

@ansibullbot
Copy link

cc @jillr @linabuzachis @lwade @s-hertel @tremble
click here for bot help

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review integration tests/integration module module needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) tests tests labels Jun 7, 2022
@tremble
Copy link
Contributor

tremble commented Jun 7, 2022

@chirag1603, thanks for taking the time to submit this PR.

This looks like an attempt to reimplement the encryption_key_id parameter from s3_bucket. aws_s3 is intended for managing objects in the bucket rather than the bucket itself. Have I misunderstood what you're trying to achieve with this change?

https://github.com/ansible-collections/amazon.aws/blob/main/plugins/modules/s3_bucket.py#L83

@zeten30
Copy link
Contributor

zeten30 commented Jun 7, 2022

@chirag1603 please modify also DOCUMENTATION= & RETURN= module sections and add some asserts into integration tests. You should verify that the particular bucket property was properly configured by the module. Thx.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

✔️ ansible-galaxy-importer SUCCESS in 4m 44s
✔️ build-ansible-collection SUCCESS in 4m 41s
ansible-test-sanity-aws-ansible-python38 FAILURE in 9m 11s
ansible-test-sanity-aws-ansible-2.9-python38 FAILURE in 11m 35s
ansible-test-sanity-aws-ansible-2.11-python38 FAILURE in 8m 15s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 5m 55s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 6m 03s
✔️ ansible-test-splitter SUCCESS in 2m 28s
integration-amazon.aws-1 FAILURE in 5m 29s
⚠️ 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-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

@zeten30
Copy link
Contributor

zeten30 commented Jun 7, 2022

@chirag1603 please take a looks at

There's a piece of code touching ServerSideEncryptionConfiguration ^. Please check if this is what you need / trying to achieve. Merge request should go to s3_bucket module instead of aws_s3.

Thanks

@tremble
Copy link
Contributor

tremble commented Jun 7, 2022

@chirag1603 thank you for taking the time to submit this PR. Looking deeper into the aws_s3 module I'm fairly certain that this PR duplicates the functionality that was added in ansible/ansible#55985 to the s3_bucket module.

Rather than adding this functionality into the aws_s3 module, duplicating code, and potentially leaving multiple places for the same bug to surface, I believe that it's better for us to let the s3_bucket module focus on creating and managing the buckets themselves, and focus the aws_s3 module focus on managing the objects (and directories) within the bucket.

As such I'm going to close this PR. However, I've also opened #869 which I hope clarifies the intended scope of this module and makes it easier to see that we have both a module for managing the S3 bucket (s3_bucket) and a module for managing the objects within the S3 bucket (s3_object).

@tremble tremble closed this Jun 7, 2022
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Jun 8, 2022
…869)

Rename aws_s3 to s3_object (and deprecate bucket creation/deleting)

SUMMARY
The aws_s3 module (as it's known today) is primarily for managing objects within S3.  While it provides minimal support for creating S3 buckets, the feature set is very limited.  Support for the advanced bucket management features is provided via the s3_bucket modules (such as managing encryption settings).
Because the name aws_s3 often puts the module at the top of the list of modules, well away from the s3_bucket module, it can be difficult for folks to discover the s3_bucket module leading them to assume that we simply have no support for the more complex s3_bucket management features.
As such, I suggest renaming the module to s3_object to make the intended scope more obvious and to improve the discoverability of s3_bucket.  At this time I do not recommend setting a deprecation date for the alias, the cost of an alias is minimal and we've had a lot of churn recently.
Additionally, deprecates the duplicated (but very limited) bucket creation/deletion functionality of aws_s3/s3_object
ISSUE TYPE

Feature Pull Request

COMPONENT NAME

aws_s3
(s3_object)

ADDITIONAL INFORMATION
See for example #866 where there was an attempt to create duplicate functionality.

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Mark Chappell <None>
Reviewed-by: Jill R <None>
jatorcasso pushed a commit to jatorcasso/amazon.aws that referenced this pull request Jun 27, 2022
…nsible-collections#869)

Rename aws_s3 to s3_object (and deprecate bucket creation/deleting)

SUMMARY
The aws_s3 module (as it's known today) is primarily for managing objects within S3.  While it provides minimal support for creating S3 buckets, the feature set is very limited.  Support for the advanced bucket management features is provided via the s3_bucket modules (such as managing encryption settings).
Because the name aws_s3 often puts the module at the top of the list of modules, well away from the s3_bucket module, it can be difficult for folks to discover the s3_bucket module leading them to assume that we simply have no support for the more complex s3_bucket management features.
As such, I suggest renaming the module to s3_object to make the intended scope more obvious and to improve the discoverability of s3_bucket.  At this time I do not recommend setting a deprecation date for the alias, the cost of an alias is minimal and we've had a lot of churn recently.
Additionally, deprecates the duplicated (but very limited) bucket creation/deletion functionality of aws_s3/s3_object
ISSUE TYPE

Feature Pull Request

COMPONENT NAME

aws_s3
(s3_object)

ADDITIONAL INFORMATION
See for example ansible-collections#866 where there was an attempt to create duplicate functionality.

Reviewed-by: Alina Buzachis <None>
Reviewed-by: Mark Chappell <None>
Reviewed-by: Jill R <None>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Oct 24, 2023
)

Update contributor doc to add changelog section

SUMMARY
Update contributor doc to add changelog section
ISSUE TYPE

Docs Pull Request

COMPONENT NAME
N/A
ADDITIONAL INFORMATION
N/A

Reviewed-by: Markus Bergholz <[email protected]>
Reviewed-by: Mark Chappell <None>
Reviewed-by: Alina Buzachis <None>
Reviewed-by: None <None>
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 module module new_contributor Help guide this first time contributor plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants