s3_object: Add parameter acl_disabled to handle uploading files to buckets with ACL disabled.
#921
Conversation
Docs Build 📝Thank you for contribution!✨ This PR has been merged and your docs changes will be incorporated when they are next published. |
|
Not sure if this is the optimal approach to move forward with this issue, keeping the PR in WIP for now to get more thoughts on it. |
ansibullbot
added
WIP
|
https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.get_bucket_ownership_controls should tell you if ACL is disabled. |
|
recheck |
There was a problem hiding this comment.
If acl_disabled isn't a user input any more it shouldn't be assigned to a module.param. I think I understand why you did it that way, because the ACL logic is currently split up and that value needs to be accessed in two different places. You could make acl_disabled a var that gets passed as an argument to upload_s3file() instead to deal with that.
I think it would be better in the long term though to take all the ACL logic (from upload_s3file() and from the series of if statements in main()) and make a new function for processing ACLs that gets called from within upload_s3file(). That way if we need to add more logic related to ACLs later it will be easier to extend or modify a single function.
Removed
Yes, it totally makes sense. I can work on another PR to handle ACLs logic once this one is merged? |
| object_ownership: BucketOwnerEnforced | ||
| state: absent | ||
| register: delete_result | ||
|
|
There was a problem hiding this comment.
I guess you should also remove the temp dir you created.
There was a problem hiding this comment.
Or better, use the helper role which adds a handler - https://github.com/ansible-collections/community.aws/blob/main/tests/integration/targets/acm_certificate/meta/main.yml
) ecs_service - document circuit breaker feature SUMMARY Fixes ansible-collections#921 This feature works with the existing code, so this was mainly adding documentation, examples, and an integration test. ISSUE TYPE Feature Pull Request COMPONENT NAME ecs_service ADDITIONAL INFORMATION The deployment circuit breaker is part of the deployment configuration dictionary, which is already snake<=>camel cased. Thus the existing code was handling 99% of the feature, we just added some type validation, documentation, examples, and an integration test. - community.aws.ecs_service: state: present name: test-service cluster: test-cluster task_definition: test-task-definition desired_count: 3 deployment_configuration: deployment_circuit_breaker: enable: True rollback: True Reviewed-by: Mark Chappell <None>
) ecs_service - document circuit breaker feature SUMMARY Fixes ansible-collections#921 This feature works with the existing code, so this was mainly adding documentation, examples, and an integration test. ISSUE TYPE Feature Pull Request COMPONENT NAME ecs_service ADDITIONAL INFORMATION The deployment circuit breaker is part of the deployment configuration dictionary, which is already snake<=>camel cased. Thus the existing code was handling 99% of the feature, we just added some type validation, documentation, examples, and an integration test. - community.aws.ecs_service: state: present name: test-service cluster: test-cluster task_definition: test-task-definition desired_count: 3 deployment_configuration: deployment_circuit_breaker: enable: True rollback: True Reviewed-by: Mark Chappell <None>
) ecs_service - document circuit breaker feature SUMMARY Fixes ansible-collections#921 This feature works with the existing code, so this was mainly adding documentation, examples, and an integration test. ISSUE TYPE Feature Pull Request COMPONENT NAME ecs_service ADDITIONAL INFORMATION The deployment circuit breaker is part of the deployment configuration dictionary, which is already snake<=>camel cased. Thus the existing code was handling 99% of the feature, we just added some type validation, documentation, examples, and an integration test. - community.aws.ecs_service: state: present name: test-service cluster: test-cluster task_definition: test-task-definition desired_count: 3 deployment_configuration: deployment_circuit_breaker: enable: True rollback: True Reviewed-by: Mark Chappell <None>
SUMMARY
Fixes #863
[Update: 07/07/2022]
https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.get_bucket_ownership_controls can be used to determine if ACL is disabled or not.
Modified code to convert
acl_disabledfrom a user input parameter to a variable used for testing if ACL is enabled/disabled.Add parameter
acl_disabledto handle uploading files to buckets with ACL disabled.If set to true, all the
permissionrelated operations are skipped in module code.ISSUE TYPE
COMPONENT NAME
s3_object
ADDITIONAL INFORMATION
AWS added the option to create S3 bucket with
ACLDisabledin Nov 2021) and made it the default/suggested setting when creating S3 bucket through AWS Portal .Currently the ACL "permission" parameter defaults to
["private"]and there is no way to tell thes3_objectmodule to omit the ACL setting while uploading files to a bucket which hasset the ACLasdisabled.Tried looking for a way to get the existing bucket info to determine if 'ACL' is
enabled/disabled, but was not able to find what I was looking for in API documentation.