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.

Sign up for GitHub

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

Jump to bottom

aws_s3: add S3 object tagging feature #335

Merged
merged 8 commits into from
Jun 14, 2021
Merged

aws_s3: add S3 object tagging feature #335

merged 8 commits into from
Jun 14, 2021

Conversation

alinabuzachis
Copy link
Collaborator

@alinabuzachis alinabuzachis commented Apr 22, 2021 •
edited by goneri
Loading

Depends-On: #372

SUMMARY

aws_s3: Add tagging feature
Fixes: #233

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

aws_s3

Requires: mattclay/aws-terminator#146

@alinabuzachis alinabuzachis mentioned this pull request Apr 22, 2021
Merged
@alinabuzachis alinabuzachis changed the title aws_s3: add S3 object tagging feature [WIP] aws_s3: add S3 object tagging feature Apr 22, 2021
@ansibullbot
Copy link

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

@ansibullbot ansibullbot added WIP Work in progress feature This issue/PR relates to a feature request integration tests/integration module module needs_triage plugins plugin (any type) tests tests labels Apr 22, 2021
jillr
jillr reviewed Apr 22, 2021
View reviewed changes
Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

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

@alinabuzachis looks like there's some logic missing, I wasn't able to test the backoff problem (do you have local changes that need to be pushed?). Here's a few I found.

plugins/modules/aws_s3.py Show resolved Hide resolved
plugins/modules/aws_s3.py Outdated Show resolved Hide resolved
plugins/modules/aws_s3.py Outdated Show resolved Hide resolved
@alinabuzachis alinabuzachis force-pushed the s3_tagging branch 2 times, most recently from df5c43c to f90db33 Compare April 23, 2021 11:34
@goneri
Copy link
Member

goneri commented Apr 23, 2021

recheck

3 similar comments
@goneri
Copy link
Member

goneri commented Apr 23, 2021

recheck

@goneri
Copy link
Member

goneri commented Apr 23, 2021

recheck

@jillr
Copy link
Collaborator

jillr commented Apr 30, 2021

recheck

@alinabuzachis alinabuzachis force-pushed the s3_tagging branch 4 times, most recently from df4ee94 to 5d47c2c Compare May 5, 2021 17:30
@alinabuzachis alinabuzachis changed the title [WIP] aws_s3: add S3 object tagging feature aws_s3: add S3 object tagging feature May 5, 2021
@alinabuzachis alinabuzachis removed the WIP Work in progress label May 6, 2021
jillr
jillr reviewed May 6, 2021
View reviewed changes
Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

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

This looks good, for put modes I think we should just call ensure_tags() after PUTing the object, rather than do a bunch of data transformation to provide the tags as Tagging='key1=value1&key2=value2',

plugins/modules/aws_s3.py Outdated Show resolved Hide resolved
plugins/modules/aws_s3.py Outdated Show resolved Hide resolved
plugins/modules/aws_s3.py Outdated Show resolved Hide resolved
@alinabuzachis
Copy link
Collaborator Author

This looks good, for put modes I think we should just call ensure_tags() after PUTing the object, rather than do a bunch of data transformation to provide the tags as Tagging='key1=value1&key2=value2',

Thank you for reviewing @jillr. Ok, I was also thinking to do it in this way.

@goneri goneri self-requested a review May 7, 2021 19:22
@goneri goneri mentioned this pull request May 7, 2021
Closed
jillr
jillr approved these changes May 14, 2021
View reviewed changes
Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

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

LGTM, @goneri did you still want to review?

@jillr
Copy link
Collaborator

jillr commented May 20, 2021

Ok to gate once the merge conflict is resolved

@goneri
Copy link
Member

goneri commented May 20, 2021

recheck

@alinabuzachis
aws_s3: Add tagging feature
20abd05 
Signed-off-by: Alina Buzachis <[email protected]>
@alinabuzachis
* Docs and logic fix
9ba4ce8 
Signed-off-by: Alina Buzachis <[email protected]>
@alinabuzachis
* Add changelog
8474067 
Signed-off-by: Alina Buzachis <[email protected]>
@alinabuzachis
* Add jittered_backoff
d39c14c 
Signed-off-by: Alina Buzachis <[email protected]>
@alinabuzachis
* Tags fix and add missming integration tests
ec2aa1d 
Signed-off-by: Alina Buzachis <[email protected]>
@alinabuzachis
* Removed unused variable
28abdd9 
Signed-off-by: Alina Buzachis <[email protected]>
@alinabuzachis
* Address reviewer's comments
b6a260f 
Signed-off-by: Alina Buzachis <[email protected]>
@alinabuzachis alinabuzachis force-pushed the s3_tagging branch 2 times, most recently from 09746ba to 2e68b5b Compare May 20, 2021 19:44
@goneri
Copy link
Member

goneri commented May 20, 2021

recheck

@jillr
Copy link
Collaborator

jillr commented May 24, 2021

recheck

4 similar comments
@goneri
Copy link
Member

goneri commented May 25, 2021

recheck

@goneri
Copy link
Member

goneri commented May 25, 2021

recheck

@goneri
Copy link
Member

goneri commented May 25, 2021

recheck

@alinabuzachis
Copy link
Collaborator Author

recheck

@alinabuzachis
Solve merge conflict
ffd8c6f 
Signed-off-by: Alina Buzachis <[email protected]>
@jillr jillr added the gate label Jun 14, 2021
@ansible-zuul ansible-zuul bot merged commit 714b3a8 into ansible-collections:main Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jillr jillr jillr approved these changes

@goneri goneri Awaiting requested review from goneri

Assignees
No one assigned
Labels
community_review feature This issue/PR relates to a feature request has_issue integration tests/integration module module plugins plugin (any type) tests tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add s3 object tagging support
4 participants
@alinabuzachis @ansibullbot @goneri @jillr