Remove key prefix from filename in recursive mode #3641
Conversation
stealthycoin
requested review from
dstufft,
jamesls,
joguSD,
JordonPhillips and
kyleknap
Codecov Report
@@ Coverage Diff @@
## v2 #3641 +/- ##
==========================================
+ Coverage 94.87% 94.88% +<.01%
==========================================
Files 177 177
Lines 13521 13516 -5
==========================================
- Hits 12828 12824 -4
+ Misses 693 692 -1
Continue to review full report at Codecov.
|
| @@ -533,6 +534,9 @@ def _display_page(self, response_data, use_basename=True): | |||
| filename = filename_components[-1] | |||
| else: | |||
| filename = content['Key'] | |||
| if strip_prefix: | |||
| if filename.startswith(strip_prefix): | |||
There was a problem hiding this comment.
Unfortunately I do not think it will be that simple. Specifically, if a user provides the following command:
$ aws s3 ls s3://mybucket/a --recurisve
It should print things like:
abc
aws-cli/__init__.py
By solely stripping out the prefix, we will start getting this instead:
bc
ws-cli/__init__.py
I tried fixing this a long time ago: #1009. It may be worth looking at the edge cases I ran into for reference. Looks like I at least put a good amount of comments for reference.
|
Also worth nothing that this removes the ability to print the full key paths. I think it could be a separate PR but we should look into adding an |
| @@ -199,6 +199,40 @@ def test_requester_pays_with_no_args(self): | |||
| 'Prefix': 'foo/' | |||
| }) | |||
|
|
|||
| def test_recursive_list_at_prefix(self): | |||
There was a problem hiding this comment.
It may be worth adding a test for the in-between delimiter cases. For example:
$ aws s3 ls s3://mybucket/f --recursive
2017-11-22 12:48:50 1448 fo
2017-11-22 12:48:50 1448 foo/bar
| if strip_prefix: | ||
| if filename.startswith(strip_prefix): | ||
| filename = filename[len(strip_prefix) + 1:] | ||
| filename = self._get_relative_key(key, content['Key']) |
There was a problem hiding this comment.
I wonder if we can simplify this method a little more? Specifically, I think we have generalized the _the_relative_key() method enough that we may be able to just remove the use_basename completely and just make the key required.
| @@ -512,7 +512,7 @@ def _list_all_objects(self, bucket, key, page_size=None, | |||
| self._display_page(response_data) | |||
|
|
|||
| def _display_page(self, response_data, use_basename=True, | |||
| strip_prefix=None): | |||
| key=''): | |||
There was a problem hiding this comment.
Would it be possible to rename this to something like provided_prefix or user_prefix? Using the name key is not necessarily correct as that value was used as the Prefix value to list objects v2 and it is a little ambiguous on where the key value came from, the response or user input.
|
@joguSD. Talked offline about this but wanted to capture what we talked about. I agree. I think the |
Removed a branch from the _display_page method and added a functional and integration test for another case.
stealthycoin
force-pushed
the
fix-recursive-output-format
branch
from
31ef7bc to
42c7b1d
Compare
stealthycoin
force-pushed
the
fix-recursive-output-format
branch
from
42c7b1d to
8cb123f
Compare
fixes #604