Add tagging option to s3 commands#6079
Conversation
vz10
force-pushed
the
s3-tagging
branch
2 times, most recently
from
b73e2ef to
925969e
Compare
Codecov Report
@@ Coverage Diff @@
## v2 #6079 +/- ##
=======================================
Coverage 93.85% 93.85%
=======================================
Files 271 271
Lines 21550 21574 +24
=======================================
+ Hits 20225 20248 +23
- Misses 1325 1326 +1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## v2 #6079 +/- ##
==========================================
- Coverage 94.20% 94.14% -0.06%
==========================================
Files 273 271 -2
Lines 21780 21580 -200
==========================================
- Hits 20518 20317 -201
- Misses 1262 1263 +1
Continue to review full report at Codecov.
|
kyleknap
left a comment
kyleknap
left a comment
There was a problem hiding this comment.
Looks like a good start. Just had some general comments/questions.
| }, | ||
| 'help_text': ( | ||
| 'The tag-set for the result object. The tag-set must be encoded ' | ||
| 'as URL Query parameters. (For example, "Key1=Value1") ' |
There was a problem hiding this comment.
Looking at the code, it looks like we are already are handling the url encoding? It might make sense to change the wording here to The tag-set must be provided as a key-value pair (e.g. "Key1=Value1,Key2=Value2")
| { | ||
| "type": "enhancement", | ||
| "category": "``s3``", | ||
| "description": "Add --tagging option to ``s3 cp`` and ``s3 sync`` commands" |
There was a problem hiding this comment.
We should also include the feature request: #2458 in the changelog so it is easy to trace back to which version it got installed in. Sort of similar to what we did in the 2.0.0 entry: https://github.com/aws/aws-cli/blob/v2/CHANGELOG.rst#200.
We should also include mention of the merge tagging feature to help trace that back too.
| if self._cli_params.get('is_move', False): | ||
| subscribers.append(DeleteCopySourceObjectSubscriber( | ||
| fileinfo.source_client)) | ||
| if self._cli_params.get('tagging', False): |
There was a problem hiding this comment.
We should probably checking on if tagging is None. Otherwise, I can see users trying to clear the tags using an empty dictionary and get confused why that does not clear the pre-existing tag set (even though they should probably just use --copy-props):
$ aws s3 cp s3://bucket/setup.py2 s3://bucket/setup.py3 --tagging "{}"
copy: s3://bucket/setup.py2 to s3://bucket/setup.py3
$ aws s3api get-object-tagging --bucket bucket --key setup.py3
{
"TagSet": [
{
"Key": "MyKey",
"Value": "foo"
},
{
"Key": "MyKe0y",
"Value": "val"
}
]
}
| self._transfer_manager.client, | ||
| self._transfer_manager.config, |
There was a problem hiding this comment.
With the CRT updates, we may have to rework how we get the client and transfer config. The CRT Transfer manager does not dangle the client nor the config off of the TransferManager (e.g. it does not have a botocore client associated to it and has its config options specified directly in its transfer client). I'll see if I can come up with ideas on how to approach this later.
| subscribers.append(DeleteCopySourceObjectSubscriber( | ||
| fileinfo.source_client)) | ||
| if self._cli_params.get('tagging', False): | ||
| subscribers.append(SetTagsSubscriber( |
There was a problem hiding this comment.
It looks like this only works for copies? I'd imagine we would want to introduce these subscribers for uploads as well?
|
|
||
|
|
||
| TAGGING_DIRECTIVE = { | ||
| 'name': 'tagging-directive', |
There was a problem hiding this comment.
What was the rationale on moving away from --merge-tagging or --merge-tags and having that be a boolean flag? I'm a little hesitant in using --tagging-directive because it shadows the API parameter and has different value and semantics, which could be confusing to customers. Also the name --merge-tagging/--merge-tags does a better job of self-documenting what it does then --tagging-directive.
There was a problem hiding this comment.
The main reason was to leave space for other merging strategies if we decide to add them
There was a problem hiding this comment.
Ah so I was thinking we would model it similar to --sse where at first it had an action of store_true and then expanded it to allow nargs of ?:
aws-cli/awscli/customizations/s3/subcommands.py
Lines 177 to 178 in 9b44abe
--merge-tagging it's presence would equate the logic for this parameter's merge. Then when we add other merge strategies, we would switch it to be nargs=? and have choices like this (we could figure out good names for them later when we implement them):
override- This is the default value for--merge-taggingaws s3 cp s3://bucket/object s3://bucket/object2 --tagging Key=value --merge-tagging overrideonly-missing- This would do the option of only adding tags that were missing from the source objectaws s3 cp s3://bucket/object s3://bucket/object2 --tagging Key=value --merge-tagging only-missingonly-existing- This would do the option of only updating tag values if the key existaws s3 cp s3://bucket/object s3://bucket/object2 --tagging Key=value --merge-tagging only-existing
How does that sound?
| for i, actual_subscriber in enumerate(actual_subscribers): | ||
| self.assertIsInstance(actual_subscriber, ref_subscribers[i]) | ||
|
|
||
| def test_tagging_option_adds_set_tag_subscriber(self): |
There was a problem hiding this comment.
It would be nice if we can get functional tests for this feature as it would serve a nice safety net in case we ever refactor all of these abstractions. I'd say we can do something similar to what we did for copy props functional tests: https://github.com/aws/aws-cli/blob/v2/tests/functional/s3/test_cp_command.py#L1349
And I say at the very least we should have a case for:
- When
--taggingis provided for copies - When
--taggingis provided for uploads (both small and multipart uploads) - When
--merge-taggingand--taggingis provided
vz10
force-pushed
the
s3-tagging
branch
3 times, most recently
from
895450b to
d2482c6
Compare
vz10
force-pushed
the
s3-tagging
branch
2 times, most recently
from
8790f79 to
c573f2b
Compare
|
Hello, any updates on merging this into the main branch for distribution? I can help test if needed. |
|
Would really appreciate if this were reviewed soon, linked issue details the use case. This has downstream impacts on CDK s3-deployment that I need to resolve. |
|
Any updates on this PR? Can we help in anyway to get this PR merged and close the #2458, opened half a decade ago? |
|
Hi ! +1, any news ? :) |
|
Any update on getting this merged in, we need to be able to create tags on objects at the time they're uploaded (cp). |
|
@kyleknap / @justindho anything? It's been open for years and no one's touched it yet. Why can't we get this merged in or reviewed? |
|
@elysahall @kyleknap , is there anything I can do to help move this PR forward ? To have the ability to put tags direcly with s3 command is an important use case for us and this PR would help us a lot |
|
@justindho any updates? This PR has been open for a while, not sure why it's taking so long? |
|
@kyleknap @stealthycoin hi guys, do we have any update on this PR approval? |
|
+1 , any update on this |
|
It's gotta be lazy devs, it's been open for a few years at this point and is a HUGE QoL change. @kyleknap are you still on GitHub? |
|
Upon bringing this PR up for discussion with the team, the decision was made to close this as not planned at this time due to higher-priority issues involving S3 commands. The team is continuing to track the corresponding feature request (#2458) and may revisit the changes proposed here. |
16 participants
Issue #, if available:
Fixes #2458
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.