-
Notifications
You must be signed in to change notification settings - Fork 487
feat(cli): migrate from argparse to Typer #2333
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
Conversation
Migrate the cartography CLI from argparse to Typer for improved UX: - Organize 100+ options into contextual help panels (AWS, Azure, GitHub, etc.) - Add dynamic help filtering: `--selected-modules aws --help` shows only relevant options (Core, Neo4j, AWS, StatsD, Analysis) - Enable shell autocompletion via `--install-completion` - Maintain 100% backward compatibility with existing CLI interface The class-based CLI structure is preserved for dependency injection and testing with mock Sync objects. Signed-off-by: Jeremy Chapeau <jeremy@music.ai> Signed-off-by: Jeremy Chapeau <jeremy@subimage.io>
- Add docs/root/usage/cli.md with full CLI reference - Update install.md to mention contextual help and shell completion - Update AGENTS.md to reflect Typer-based CLI Signed-off-by: Jeremy Chapeau <jeremy@music.ai> Signed-off-by: Jeremy Chapeau <jeremy@subimage.io>
- Update create-module.md with Typer option syntax and help panels - Update troubleshooting.md CLI reference - Update writing-intel-modules.md with current CLI patterns Signed-off-by: Jeremy Chapeau <jeremy@music.ai> Signed-off-by: Jeremy Chapeau <jeremy@subimage.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 8 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="docs/agents/create-module.md">
<violation number="1" location="docs/agents/create-module.md:543">
P2: The new guidance passes the API key value into Config, but the later validation snippet still treats `config.your_service_api_key` as an environment variable name. Update one of these sections so the docs are internally consistent (e.g., use `config.your_service_api_key` directly in validation or keep Config storing the env var name).</violation>
</file>
<file name="docs/root/usage/index.md">
<violation number="1" location="docs/root/usage/index.md:4">
P2: The new `cli` toctree entry points to a missing document. Add the corresponding `docs/root/usage/cli.*` file (or adjust the toctree entry to the correct path) so the Sphinx build doesn’t fail with a missing document.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Update test_permission_relationships_file_arguments to work with Typer: - Remove dependency on cli.parser (argparse-specific) - Mock cartography.sync.run_with_config to capture config - Split into two tests for clarity Signed-off-by: Jeremy Chapeau <jeremy@music.ai> Signed-off-by: Jeremy Chapeau <jeremy@subimage.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="tests/integration/cartography/intel/aws/iam/test_iam.py">
<violation number="1" location="tests/integration/cartography/intel/aws/iam/test_iam.py:51">
P1: Rule violated: **Tests and documentation quality**
Integration tests must validate graph outcomes and should not mock Cartography internals. This test mocks cartography.sync.run_with_config (an internal function) and asserts parameters passed to the mock instead of checking graph data, violating the “Mock only external APIs” and “Test outcomes, not implementation” clauses.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
The validation example incorrectly treated config.your_service_api_key as an environment variable name instead of the resolved value. The CLI already reads the env var and passes the resolved value to Config. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Jeremy Chapeau <jeremy@subimage.io>
kunaals
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks almost good to go! i just see one issue: spacelift_ec2_ownership_aws_profile is missing from Config: the CLI option is defined but the value is never passed to the Config object
im not going to stamp just yet i think @achantavy should probably stamp this one
Add the missing spacelift_ec2_ownership_aws_profile parameter to the Config class and pass it from the CLI. The CLI option was defined but the value was never passed to the Config object. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Jeremy Chapeau <jeremy@subimage.io>
achantavy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor things but this is super exciting.
just on sync.py we can update the typehints and imports to clean up a bit now that things are simplified and explicit
| functions like cartography-rules does) for backward compatibility. The existing | ||
| codebase and tests rely on being able to: | ||
|
|
||
| 1. Inject a custom Sync object: `CLI(sync=my_custom_sync)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very cool
cartography/cli.py
Outdated
| ), | ||
| ] = None, | ||
| aws_requested_syncs: Annotated[ | ||
| Optional[str], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: new annotation style e.g. str | None
| ) | ||
|
|
||
| # Build the Config object | ||
| config = cartography.config.Config( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously we were taking the raw argparse Namespace object and passing that to sync.py's run_with_config. Now that we're explicitly creating a more type-safe Config object, we can update sync.py to remove the argparse.Namespace import and simplify the type hints.
Address review feedback: - Replace Optional[X] with X | None style throughout cli.py - Remove argparse import from sync.py (no longer needed) - Simplify type hints in sync.py to use Config directly - Use lowercase list/tuple for type hints per Python 3.10+ style Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Jeremy Chapeau <jeremy@subimage.io>


Type of change
Summary
Migrate the Cartography CLI from argparse to Typer for improved user experience.
Key improvements:
cartography --selected-modules aws --helpshows only AWS-related options, making it easier to discover relevant parameters--install-completionImplementation details:
--selected-modulesfrom argv before building the Typer app to enable dynamic help filteringhidden=Trueon options whose panels aren't relevant to selected modulesRelated issues or links
--helpoutputcartography-ruleswhich already uses TyperNote: typer allow configuration from var env that we will be able to enable in a following PR.
How was this tested?
make test_lint)tests/integration/cartography/test_cli.pypassesChecklist
General
make lint).Proof of functionality
Notes for reviewers
This is a large diff due to the migration from argparse to Typer, but the logic is straightforward:
typer.Option()withrich_help_panelfor groupinghiddenparameter enables dynamic help filtering based on--selected-modulesThe contributor documentation has been updated to show the new Typer patterns for adding CLI options.