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

[v2] Expose --bucket-name-prefix and --bucket-region to s3 ls #9163

Merged
merged 5 commits into from
Jan 6, 2025

Conversation

hssyoo
Copy link
Contributor

@hssyoo hssyoo commented Dec 23, 2024

S3's ListBuckets API released filter parameters Prefix and BucketRegion. This PR exposes the low-level parameters to the high-level s3 ls command by exposing the --bucket-name-prefix and --bucket-region parameters. These parameters are only used if no S3 URI is supplied to the command:

  • aws s3 ls --bucket-name-prefix foo is fine.
  • aws s3 ls s3://mybucket --bucket-name-prefix foo is functionally the same as aws s3 ls s3://mybucket.

Decisions made:

  • Map Prefix to --bucket-name-prefix. s3 ls calls ListObjectsV2 under the hood if an S3 URI is supplied. In that case, the bucket name and prefix/key are parsed, and the Prefix parameter is automatically (or implicitly) supplied. Without an S3 URI, ListBuckets is called and the Prefix parameter must be explicitly supplied by the high-level s3 ls parameter. To avoid confusion, ListBuckets's Prefix parameter is exposed as --bucket-name-prefix.
  • The client doesn't attempt to validate that the 2 new parameters are only used when an S3 URI is not supplied. So aws s3 ls s3://mybucket --bucket-region us-east-1 does not throw an error/warning.
    1. This is consistent with how parameters are generally handled for high-level s3 commands.
    2. The parameter names clearly refer to buckets (not objects) and additional details are available in the help docs.

v1 PR will be opened after this is approved.

@hssyoo hssyoo requested review from kdaily, ashovlin and aemous December 23, 2024 17:54
Copy link
Contributor

@aemous aemous left a comment

Choose a reason for hiding this comment

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

Looks good overall. I'd like to a see a unit & functional tests against specifying one of these args with an S3 URI, to verify it ignores the arg.

@hssyoo hssyoo requested a review from aemous January 6, 2025 15:15
Copy link
Contributor

@aemous aemous left a comment

Choose a reason for hiding this comment

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

Looks good to me

@hssyoo hssyoo merged commit d4619a3 into aws:v2 Jan 6, 2025
45 checks passed
@hssyoo hssyoo deleted the ls-filters-v2 branch January 6, 2025 21:47
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (c9d26de) to head (abc78a4).
Report is 93 commits behind head on v2.

Additional details and impacted files
@@    Coverage Diff     @@
##   v2   #9163   +/-   ##
==========================
==========================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants