-
Notifications
You must be signed in to change notification settings - Fork 487
fix(cli): add version flags, restore -h, and speed up --help #2367
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
Signed-off-by: Jeremy Chapeau <jeremy@subimage.io>
Signed-off-by: Jeremy Chapeau <jeremy@subimage.io>
Signed-off-by: Jeremy Chapeau <jeremy@subimage.io>
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 6 files (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/unit/cartography/graph/test_matchlink.py">
<violation number="1" location="tests/unit/cartography/graph/test_matchlink.py:19">
P2: Rule violated: **Tests and documentation quality**
This test mocks an internal Cartography function (get_cartography_version), which violates the rule to mock only external APIs. Update the test to avoid patching internal helpers and instead assert against the real version value or another non-mocked mechanism.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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.
3 issues found across 4 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="cartography/cli.py">
<violation number="1" location="cartography/cli.py:119">
P2: `--version` raises `typer.Exit`, but with `standalone_mode=False` Click doesn’t translate it into `SystemExit`, so it falls into the generic exception handler and returns `STATUS_FAILURE`. That makes `cartography --version` exit with a failure code. Consider raising `SystemExit(0)` here (or handle `typer.Exit` explicitly in `main`).</violation>
</file>
<file name="tests/integration/cartography/test_cli.py">
<violation number="1" location="tests/integration/cartography/test_cli.py:24">
P2: Rule violated: **Tests and documentation quality**
These new integration tests assert mock call counts on `sync.run`, which violates the "Test outcomes, not implementation" requirement that tests verify graph data (via check_nodes/check_rels) rather than mock interactions. Update these tests to validate resulting graph state (or other real outcomes) instead of mock call assertions.</violation>
</file>
<file name="cartography/version.py">
<violation number="1" location="cartography/version.py:20">
P1: Rule violated: **General coding rules**
Do not swallow PackageNotFoundError with a fallback return. The general coding rule requires letting exceptions bubble up instead of masking them with defaults.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Signed-off-by: Jeremy Chapeau <jeremy@subimage.io>
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.
3 issues found across 4 files (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="cartography/version.py">
<violation number="1" location="cartography/version.py:10">
P2: Returning package metadata without handling PackageNotFoundError will crash in source/dev environments where metadata is missing. Reintroduce the fallback so version lookup failure doesn’t abort the CLI.</violation>
</file>
<file name="tests/integration/cartography/test_cli.py">
<violation number="1" location="tests/integration/cartography/test_cli.py:55">
P2: Rule violated: **Tests and documentation quality**
Avoid mocking Cartography’s internal functions in tests. This test patches the internal `_build_app` method, which violates the “Mock only external APIs” requirement. Use the real CLI app behavior (e.g., invoke a real Typer app path that exits) or restructure the test so only external API calls are mocked.</violation>
</file>
<file name="tests/unit/cartography/graph/test_matchlink.py">
<violation number="1" location="tests/unit/cartography/graph/test_matchlink.py:18">
P1: Rule violated: **Tests and documentation quality**
Avoid mocking Cartography internal functions in tests; only external API calls should be mocked per “Mock only external APIs.” Replace this with a test that exercises the real `get_cartography_version` behavior (or refactor the test to avoid patching internal helpers).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Signed-off-by: Jeremy Chapeau <jeremy@subimage.io>
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 2 files (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="cartography/version.py">
<violation number="1" location="cartography/version.py:22">
P1: Rule violated: **General coding rules**
Do not suppress PackageNotFoundError here. The rule requires errors to bubble up rather than being caught and replaced with fallback values.</violation>
</file>
<file name="tests/unit/cartography/test_version.py">
<violation number="1" location="tests/unit/cartography/test_version.py:10">
P1: Rule violated: **Tests and documentation quality**
Mocking Cartography’s internal `get_release_version_and_commit_revision` violates the “Mock only external APIs” clause in Tests and documentation quality. Rewrite this test to avoid mocking internal helpers (e.g., assert against real version behavior or only mock external API calls).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@jychp - How long does |
Type of change
Summary
--versionoutput to the main CLI.-v/--verbosebehavior and adds-d/--debugas an alias to the same option.--verboseas deprecated in help text (planned removal inv1.0.0).-halongside--help.cartography --helpstartup by lazy-loading heavy sync/validation imports and deferring default sync construction until command execution.Related issues or links
cartography -hno longer works after PR #2333 #2364cartography --helpis very slow #2365Breaking changes
None.
How was this tested?
UV_CACHE_DIR=/tmp/uv-cache uv run pytest -q tests/integration/cartography/test_cli.pyUV_CACHE_DIR=/tmp/uv-cache uv run pytest -q tests/integration/cartography/intel/aws/iam/test_iam.py::test_permission_relationships_file_clitime UV_CACHE_DIR=/tmp/uv-cache uv run cartography --helpChecklist
General
make lint).Proof of functionality
If you are adding or modifying a synced entity
If you are changing a node or relationship
If you are implementing a new intel module
Notes for reviewers
cartography/sync.py).