Skip to content

Add profile as an option to every click command #12500

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Apr 15, 2025
Merged

Add profile as an option to every click command #12500

merged 8 commits into from
Apr 15, 2025

Conversation

jw2
Copy link
Contributor

@jw2 jw2 commented Apr 8, 2025 •
edited
Loading

Motivation

We've had feedback that mandating the --profile option be specified at the top level, before any actions, is overly restrictive and causes headaches when wrapping the CLI in custom developer tools. The following currently works:

localstack --profile=custom start

but the following:

localstack start --profile=custom

fails with:

Usage: localstack start [OPTIONS]
Try 'localstack start -h' for help.

Error: No such option: --profile

Changes

--profile can now be specified at any point. This is achieved by removing any occurrences of --profile options from the command line arguments (sys.argv) before click gets invoked.

The profile option was already being read and used before click was invoked, and click performed no further processing of it. The only functionality that click was actually performing w.r.t. the profile option was:

  1. ensuring it was listed in the help
  2. ensuring that it can only be provided as a top-level option (which we don't want).

--profile no longer makes it through to click, so 2. never happens.

There is no change to the processing of -p because that option is used in at least one subcommand. Therefore it would break those options if we were to strip it out like --profile. Ideally, we would remove the ability to specify a profile using -p as there is a pre-existing issue whereby if a port mapping command was specified, e.g. localstart start -p 80:8080, then localstack would start using a profile called 80:8080. However, this would break back-compatibility.

There is another top-level switch --debug and this remains top-level only. The reasoning behind this decision is:

  • There is no known requirement to allow this to be specified anywhere.
  • The fix would be more difficult - either it would need to be added to every possible command, or further effort to extract the processing outside of click like --profile.

@jw2 jw2 self-assigned this Apr 8, 2025
@jw2 jw2 added the semver: patch Non-breaking changes which can be included in patch releases label Apr 8, 2025
@jw2 jw2 force-pushed the common-cli-options branch from 138ed76 to a1118c1 Compare April 9, 2025 16:34
jw2 added 2 commits April 9, 2025 17:52
@jw2
Fix lint errors
4830786
@jw2
Change function names to something more appropriate
4a0245f 
Add warnings to comments that sys.argv is modified.
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 9, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 52m 24s ⏱️ -41s
4 350 tests +4  4 004 ✅ +25  346 💤  - 15  0 ❌ ±0 
4 352 runs  +4  4 004 ✅ +25  348 💤  - 15  0 ❌ ±0 

Results for commit 3d21af5. ± Comparison against base commit 69d7a75.

This pull request removes 1 and adds 5 tests. Note that renamed tests count towards both. 
tests.aws.services.ec2.test_ec2.TestEc2Integrations ‑ test_create_security_group_with_custom_id

tests.aws.services.ec2.test_ec2.TestEc2Integrations ‑ test_create_security_group_with_custom_id[False-id_manager]
tests.aws.services.ec2.test_ec2.TestEc2Integrations ‑ test_create_security_group_with_custom_id[False-tag]
tests.aws.services.ec2.test_ec2.TestEc2Integrations ‑ test_create_security_group_with_custom_id[True-id_manager]
tests.aws.services.ec2.test_ec2.TestEc2Integrations ‑ test_create_security_group_with_custom_id[True-tag]
tests.aws.services.kms.test_kms.TestKMS ‑ test_create_custom_key_asymmetric

♻️ This comment has been updated with latest results.

@jw2 jw2 marked this pull request as ready for review April 10, 2025 05:56
@jw2 jw2 requested review from thrau and alexrashed as code owners April 10, 2025 05:56
thrau
thrau approved these changes Apr 10, 2025
View reviewed changes
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.

I think this is a decent solution for now. Clean implementation and well documented 💯 Thank you Jim!

alexrashed
alexrashed approved these changes Apr 10, 2025
View reviewed changes
Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

I think this is a great solution! Even though manipulating the sys.argv is a bit hacky, it is the cleanest solution considering that (a) we have to evaluate the profile before we load the config module and (b) there is no native support for global arugments in click (pallets/click#108)! 💯
Clean implementation, nice unit tests, clear documentation! 🦸🏽

The failing checks in your PR are due to flaky transcribe tests which have already been marked to skip on master with #12509.
Imho this is ready to be merged! :shipit: 🥳

@jw2
Copy link
Contributor Author

jw2 commented Apr 14, 2025

I've reworked this PR as follows.

  1. Daniel pointed out I could use https://docs.python.org/3/library/argparse.html#argparse.ArgumentParser.parse_known_args to replace the manual parsing I'd written.
  2. There was an issue with stripping out -p parameters from the arguments when these could be genuine non-profile arguments to subcommands, e.g. localstack start -p 80:8080 to pass a port range to docker. As such, I've changed the PR so that only --profile can appear anywhere on the command line.

Note the updated PR description which discussed the existing bugged behaviour of -p. One other option might be to deprecate use of -p for specifying a profile.

@alexrashed could you give this another check please seeing as the PR is substantially changed?

@jw2 jw2 requested a review from alexrashed April 14, 2025 08:33
alexrashed
alexrashed approved these changes Apr 14, 2025
View reviewed changes
Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the issue with the abbreviation / -p! This conflict could really cause a bit of confusion. I think it's fair to just allow --profile on all levels. The implementation and the tests are looking good! 💯

@jw2 jw2 merged commit a86bbca into master Apr 15, 2025
31 checks passed
@jw2 jw2 deleted the common-cli-options branch April 15, 2025 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexrashed alexrashed alexrashed approved these changes

@thrau thrau thrau approved these changes

Assignees

@alexrashed alexrashed

@jw2 jw2

Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jw2 @alexrashed @thrau