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

add new CLI command to simplify shell completion #8413

Merged
merged 2 commits into from Jun 5, 2023

Conversation

alexrashed
Copy link
Member

@alexrashed alexrashed commented Jun 1, 2023

This PR adds a new shell command to support the configuration of auto-completion for the LocalStack CLI.
It uses the Click-feature for Shell completion: https://click.palletsprojects.com/en/8.1.x/shell-completion/

Unfortunately, the configuration is a bit cumbersome:

In order for completion to be used, the user needs to register a special function with their shell. The script is different for every shell, and Click will output it when called with _{PROG_NAME}_COMPLETE set to {shell}_source. {PROG_NAME} is the executable name in uppercase with dashes replaced by underscores. The built-in shells are bash, zsh, and fish.

This is where the new completion command comes in. It basically just simplifies printing the shell completion code for a given shell. This completion code can then - depending on the shell - be used to configure the shell completion.
By implementing a specific command, it's also easy to ship an extensive documentation explaining the feature (as you can see in the code).

When starting to work on this, I used click-contrib/click-completion for a first PoC. It would even have an install command and powershell support, but I decided against it because:

  • It would have added Jinja2 as an install dependency.
  • It is not too well maintained, there hasn't been a proper release for quite some time (Click >= 8 is not supported in the latest release on PyPi).
  • It seem like the powershell support might be broken right now, and click allows to easily add new completions.

@alexrashed alexrashed added area: cli LocalStack CLI semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Jun 1, 2023
@alexrashed alexrashed requested a review from thrau as a code owner June 1, 2023 12:06
Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

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

this looks like a really nice quality of life improvement! i wonder whether we should maybe group certain commands into utility categories, such as localstack cfg localstack cfg-cli localstack cli or something of sorts, to keep the list of top-level commands short and make sure they focus on the core functionality.
WDYT? /cc @SimonWallner

@alexrashed
Copy link
Member Author

That's a good point... There is already the config command, but that seems semantically closer to the actual configuration of the runtime / providing tools to handle the runtime config.
But I think it might also be good to have it on the top level because:

  • It seems to be quite common:
  • I don't really see other commands exposed for the CLI per-se. I think other commands, which are somewhat related to the CLI like login will still stay on the top level?

Do you think there are going to be other commands which would semantically belong to this command?
Maybe we should also hide it in the list of commands like the docker cli does?

@alexrashed alexrashed changed the base branch from cli-improvements to master June 1, 2023 14:25
@github-actions
Copy link

github-actions bot commented Jun 1, 2023

LocalStack Community integration with Pro

2 082 tests   1 799 ✔️  1h 21m 14s ⏱️
       2 suites     283 💤
       2 files           0

Results for commit af6e2a6.

♻️ This comment has been updated with latest results.

@coveralls
Copy link

coveralls commented Jun 2, 2023

Coverage Status

coverage: 82.729% (+0.05%) from 82.68% when pulling af6e2a6 on add-cli-complete-command into 71178ff on master.

@alexrashed
Copy link
Member Author

FYI: The failing Community Integration Tests against Pro are addressed in #8426. But these tests should not be affected by the changes in this PR.

Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

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

nice! since it seems industry practice, let's do it 👍

@alexrashed alexrashed merged commit 30f2123 into master Jun 5, 2023
24 checks passed
@alexrashed alexrashed deleted the add-cli-complete-command branch June 5, 2023 14:44
@dfangl dfangl added this to the 2.2 milestone Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: cli LocalStack CLI semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants