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

Include global options using pre-generated rst files #7157

Merged
merged 1 commit into from
Aug 19, 2022

Conversation

hssyoo
Copy link
Contributor

@hssyoo hssyoo commented Aug 2, 2022

We get a lot of requests from users to include global options in a command's help page. The changes in this PR include:

  • A script used to pre-generate .rst files for global options
  • The inclusion of the global options in help docs
  • A pair of tests to ensure that the generated .rst files are up to date

Preview of doc changes
image

image

@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2022

Codecov Report

Merging #7157 (7f9ac3d) into develop (ff9b332) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #7157      +/-   ##
===========================================
+ Coverage    92.89%   92.91%   +0.02%     
===========================================
  Files          204      205       +1     
  Lines        16362    16416      +54     
===========================================
+ Hits         15199    15253      +54     
  Misses        1163     1163              
Impacted Files Coverage Δ
awscli/bcdoc/docevents.py 100.00% <100.00%> (ø)
awscli/bcdoc/restdoc.py 99.13% <100.00%> (+0.03%) ⬆️
awscli/clidocs.py 99.24% <100.00%> (+0.03%) ⬆️
awscli/customizations/commands.py 99.56% <100.00%> (ø)
awscli/handlers.py 100.00% <0.00%> (ø)
awscli/customizations/rds.py 100.00% <0.00%> (ø)
awscli/customizations/overridesslcommonname.py 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

It's looking good! I definitely like this approach over all the other ones we considered. This first round of feedback was more around usage/style patterns and interfaces. I did not look at the tests/script or look to deeply through the content generated for the various command types, but I'll look at that in the next pass.

awscli/clidocs.py Outdated Show resolved Hide resolved
awscli/clidocs.py Outdated Show resolved Hide resolved
awscli/clidocs.py Outdated Show resolved Hide resolved
awscli/clidocs.py Outdated Show resolved Hide resolved
awscli/examples/global/global_synopsis.rst Outdated Show resolved Hide resolved
awscli/clidocs.py Outdated Show resolved Hide resolved
awscli/clidocs.py Outdated Show resolved Hide resolved
awscli/clidocs.py Outdated Show resolved Hide resolved
awscli/clidocs.py Outdated Show resolved Hide resolved
awscli/clidocs.py Outdated Show resolved Hide resolved
awscli/bcdoc/restdoc.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

It's looking good. I just had some more comments as we polish this.

awscli/clidocs.py Outdated Show resolved Hide resolved
scripts/document-globals Outdated Show resolved Hide resolved
awscli/examples/global_options.rst Outdated Show resolved Hide resolved
awscli/clidocs.py Outdated Show resolved Hide resolved
tests/functional/test_globals.py Outdated Show resolved Hide resolved
tests/functional/test_globals.py Outdated Show resolved Hide resolved
tests/functional/test_globals.py Show resolved Hide resolved
awscli/bcdoc/restdoc.py Outdated Show resolved Hide resolved
awscli/clidocs.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kyleknap kyleknap 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. I think we are getting close. I just had some more comments.

awscli/clidocs.py Outdated Show resolved Hide resolved
tests/unit/test_clidocs.py Outdated Show resolved Hide resolved
tests/unit/test_clidocs.py Outdated Show resolved Hide resolved
awscli/bcdoc/restdoc.py Outdated Show resolved Hide resolved
awscli/clidocs.py Show resolved Hide resolved
Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Looks fine. I just had some more comments based on the updates.

awscli/bcdoc/restdoc.py Outdated Show resolved Hide resolved
tests/unit/test_clidocs.py Outdated Show resolved Hide resolved
tests/unit/customizations/test_commands.py Outdated Show resolved Hide resolved
tests/functional/docs/test_help_output.py Show resolved Hide resolved
awscli/customizations/commands.py Outdated Show resolved Hide resolved
awscli/clidocs.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kyleknap kyleknap 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. Just some more more smaller comments mostly around the tests.

tests/unit/test_clidocs.py Outdated Show resolved Hide resolved
tests/unit/customizations/test_commands.py Outdated Show resolved Hide resolved
tests/unit/customizations/test_commands.py Outdated Show resolved Hide resolved
awscli/customizations/commands.py Outdated Show resolved Hide resolved
awscli/clidocs.py Show resolved Hide resolved
Copy link
Contributor

@kyleknap kyleknap 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! Let's go ahead squash the commit and reformat the commit message and we should be set to merge this.

@hssyoo hssyoo force-pushed the include-global-options-rst branch 4 times, most recently from e541d42 to 5e82043 Compare August 18, 2022 23:46
We get a lot of requests from users to include
global options in a command's help page. This PR
fulfils them by pre-generating .rst files for
global options and synopsis and writing them to
commands' help docs.

An alternative that was considered attempted to
plumb the global arg table from the CLIDriver to
any help command that would need it. However, we
abandoned the approach because it was too invasive
to the existing interfaces and there was no easy
way to apply the changes to customizations.
Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Looks great! 🚢 Merging

@kyleknap kyleknap merged commit d1bf9e6 into aws:develop Aug 19, 2022
TheRealAmazonKendra added a commit to aws/aws-cdk that referenced this pull request Aug 22, 2022
[This change in the aws cli](aws/aws-cli#7157) made --help use
global_options.rst in the examples folder. We need to keep that file so that running /opt/awscli/aws help
doesn't break.
mergify bot pushed a commit to aws/aws-cdk that referenced this pull request Aug 23, 2022
…21716)

[This change in the aws cli](aws/aws-cli#7157) made the `help` command use 
the file `global_options.rst` in the `opt/awscli/awscli/examples` folder. 
We need to keep that file so that running 
`/opt/awscli/aws help` doesn't break.

I moved this into a new PR from the dependabot one because I didn't want it to auto-approve my changes.

----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mergify bot pushed a commit to aws/aws-cdk that referenced this pull request Aug 23, 2022
…21716)

[This change in the aws cli](aws/aws-cli#7157) made the `help` command use
the file `global_options.rst` in the `opt/awscli/awscli/examples` folder.
We need to keep that file so that running
`/opt/awscli/aws help` doesn't break.

I moved this into a new PR from the dependabot one because I didn't want it to auto-approve my changes.

----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

(cherry picked from commit 5ce0a09)
josephedward pushed a commit to josephedward/aws-cdk that referenced this pull request Aug 30, 2022
…ws#21716)

[This change in the aws cli](aws/aws-cli#7157) made the `help` command use 
the file `global_options.rst` in the `opt/awscli/awscli/examples` folder. 
We need to keep that file so that running 
`/opt/awscli/aws help` doesn't break.

I moved this into a new PR from the dependabot one because I didn't want it to auto-approve my changes.

----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@hssyoo hssyoo deleted the include-global-options-rst branch September 16, 2022 14:36
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.

3 participants