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

AWS help + docs.aws.amazon.com - --storage-class options should be sorted #6608

Closed
uri-rodberg opened this issue Dec 10, 2021 · 9 comments
Closed
Labels
documentation This is a problem with documentation. feature-request A feature should be added or improved.

Comments

@uri-rodberg
Copy link

uri-rodberg commented Dec 10, 2021

When typing aws s3 cp help with aws-cli/2.4.6 I receive the following text:

       --storage-class  (string)  The  type  of storage to use for the object.
       Valid choices are: STANDARD | REDUCED_REDUNDANCY | STANDARD_IA  |  ONE-
       ZONE_IA  |  INTELLIGENT_TIERING  | GLACIER | DEEP_ARCHIVE | GLACIER_IR.
       Defaults to 'STANDARD'

A similar text also appears on https://docs.aws.amazon.com/cli/latest/reference/s3/cp.html

I think the storage classes options should be sorted according to their order on https://aws.amazon.com/s3/storage-classes/?nc=sn&loc=3, and not as they are currently sorted. Specifically, GLACIER_IR should come before GLACIER.

Is your feature request related to a problem? Please describe.

Describe the solution you'd like

Describe alternatives you've considered

Additional context

This text should appear both on aws s3 cp help and on https://docs.aws.amazon.com/cli/latest/reference/s3/cp.html
Also aws s3 sync help, https://docs.aws.amazon.com/cli/latest/reference/s3/sync.html etc.

@uri-rodberg uri-rodberg added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Dec 10, 2021
@kdaily kdaily self-assigned this Dec 10, 2021
@kdaily kdaily added documentation This is a problem with documentation. investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Dec 10, 2021
@kdaily
Copy link
Member

kdaily commented Dec 10, 2021

Hi @uri-rodberg,

Thanks for the suggestion! This specific parameter is configured manually. I think for consistency we would need to apply to all parameters that have a limited set of options (they're an enum type in the API model). I'll look into this!

@kdaily
Copy link
Member

kdaily commented Dec 10, 2021

I think we may be able to change this specific parameter after consulting with the docs team.

For a more general solution, we'd need to do some more research. First, the default parameter should probably be listed first. Second, we would need to confirm that the values are not manually ordered with some other intention by service team doc writers.

@kdaily kdaily removed the investigating This issue is being investigated and/or work is in progress to resolve the issue. label Dec 10, 2021
@kdaily kdaily removed their assignment Dec 10, 2021
@uri-rodberg
Copy link
Author

First, the default parameter should probably be listed first.

Yes. I agree. In this case the default storage-class (STANDARD) is first.

@uri-rodberg
Copy link
Author

I think the source code for this feature is here:
https://github.com/aws/aws-cli/blob/develop/awscli/customizations/s3/subcommands.py#L249-L258

@kdaily
Copy link
Member

kdaily commented Dec 11, 2021

@uri-rodberg,

Yep! We just added GLACIER_IR to that list recently as well, and added it to the end.

Looks like it needs to be changed in two places there (the list and the string).

Feel free to open a PR for that - I'll still need to discuss with the S3 docs team, so no guarantees on when it could be merged, but we would appreciate your contribution.

@uri-rodberg
Copy link
Author

When editing storage class in S3 console, this is the order of classes:

Standard
Intelligent-Tiering
Standard-IA
One Zone-IA
Glacier Instant Retrieval
Glacier Flexible Retrieval (formerly Glacier)
Glacier Deep Archive
Reduced redundancy

@kdaily
Copy link
Member

kdaily commented Apr 7, 2022

Hi @uri-rodberg,

Thanks again for your patience. After review and conversations with the S3 team, we decided not to make this change here. We want to avoid the scenario where other storage classes are added in the future and reordering is necessary again. The change should be made in the API model, and the order in the model is the same as we currently list:

https://github.com/boto/botocore/blob/4555c2f9c82c051eb7b317c4458a22b4cccf293b/botocore/data/s3/2006-03-01/service-2.json#L7203-L7216

The S3 documentation team would also like consistency between the order in the documentation and the model, so we'll work internally to try and change the ordering, but I don't have a timeline when that would occur. After that, we'd look at how to make sure the high level S3 commands also use that ordering and add some testing around it.

Again, thanks for bringing this to our attention and I apologize for not coming to a decision sooner. We are currently working on improving our process around community contributions with a public proposal here. You can leave any feedback you have about your experience or the proposed process on this pull request. Thanks!

@kdaily kdaily closed this as completed Apr 7, 2022
@github-actions
Copy link

github-actions bot commented Apr 7, 2022

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.

@uri-rodberg
Copy link
Author

OK, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This is a problem with documentation. feature-request A feature should be added or improved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants