feat: add GCP labels and tags support to CLI commands#30
Merged
Conversation
Add opt-in support for GCP resource labels (key-value pairs) and resource tags (Tag Manager bindings) across CLI commands. Labels are fetched via an additional SQL column in Asset API queries (cheap). Tags require a separate Asset API query against TagBinding resources (expensive). Both are only fetched when explicitly requested via CLI flags. New CLI options on ls, tree, find, ancestors: - --show-labels: display labels in output - --show-tags: display tags in output - --label key=value: filter by label (repeatable, ANDed) - --tag key=value: filter by tag (repeatable, ANDed) Changes across the stack: - core.py: labels/tags fields on Folder and Project dataclasses - parsers.py: extract_labels() and has_labels param on parse functions - loaders.py: include_labels in SQL builders, load_tags_asset/apply_tags - cache.py: bump CACHE_VERSION to 2, serialize/deserialize labels+tags - serializers.py: include labels/tags in JSON/YAML output when non-empty - formatters.py: show labels/tags in tree view labels https://claude.ai/code/session_01HugtU9fbaL97tbb7zaqNPL
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds labels and tags end-to-end: Folder/Project dataclasses gain Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as "CLI"
participant Core as "Core\n(Hierarchy.load)"
participant Cache as "Cache"
participant Loaders as "Loaders"
participant AssetAPI as "Asset API"
User->>CLI: run command (e.g. --show-labels --label key=val)
CLI->>Core: Hierarchy.load(include_labels=True, include_tags=True)
Core->>Cache: check cached hierarchy (version)
alt cache hit v2
Cache-->>Core: cached hierarchy (with labels/tags)
else cache miss or version mismatch
Core->>Loaders: load_projects_asset(include_labels=True)
Loaders->>AssetAPI: query projects (+labels)
AssetAPI-->>Loaders: project rows
Loaders->>Loaders: parse_project_row(has_labels=True)
Loaders-->>Core: projects (labels set)
Core->>Loaders: load_tags_asset(parent)
Loaders->>AssetAPI: query TagBinding
AssetAPI-->>Loaders: tag binding rows
Loaders-->>Core: tags_map
Core->>Loaders: apply_tags(hierarchy, tags_map)
Core->>Cache: serialize and save hierarchy (version 2)
end
Core-->>CLI: Hierarchy with labels & tags
CLI->>CLI: filter by --label/--tag and render with --show-labels/--show-tags
CLI-->>User: display filtered results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #30 +/- ##
==========================================
- Coverage 89.55% 87.29% -2.27%
==========================================
Files 9 9
Lines 1637 1786 +149
==========================================
+ Hits 1466 1559 +93
- Misses 171 227 +56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for GCP labels and tags across the gcpath tool. It updates the core data models, CLI commands (ls, tree, find, ancestors), and the caching mechanism (bumping the cache version to 2) to handle these metadata fields. Users can now display labels and tags in various output formats and filter resources using the new --label and --tag options. Review feedback highlights that the label and tag options for the ancestors command are currently unimplemented and suggests refactoring duplicate filtering and formatting logic into generic helper functions.
Comment on lines
+1361
to
+1366
| show_labels: bool = typer.Option( | ||
| False, "--show-labels", help="Display GCP labels on resources" | ||
| ), | ||
| show_tags: bool = typer.Option( | ||
| False, "--show-tags", help="Display GCP resource tags" | ||
| ), |
Contributor
There was a problem hiding this comment.
The --show-labels and --show-tags options are added to the ancestors command, but they don't appear to be implemented. The command output does not include labels or tags, and the underlying Hierarchy.resolve_ancestry_chain method doesn't fetch this information. These options should either be fully implemented or removed to avoid user confusion.
Comment on lines
+93
to
+145
| def _matches_labels( | ||
| obj: Union[OrganizationNode, Folder, Project], | ||
| label_filters: List[str], | ||
| ) -> bool: | ||
| """Check if a resource matches ALL label filters (ANDed).""" | ||
| if not label_filters: | ||
| return True | ||
| labels = getattr(obj, "labels", {}) | ||
| for lf in label_filters: | ||
| key, value = _parse_label_filter(lf) | ||
| if value is None: | ||
| # Key-only filter: check if key exists | ||
| if key not in labels: | ||
| return False | ||
| else: | ||
| if labels.get(key) != value: | ||
| return False | ||
| return True | ||
|
|
||
|
|
||
| def _matches_tags( | ||
| obj: Union[OrganizationNode, Folder, Project], | ||
| tag_filters: List[str], | ||
| ) -> bool: | ||
| """Check if a resource matches ALL tag filters (ANDed).""" | ||
| if not tag_filters: | ||
| return True | ||
| tags = getattr(obj, "tags", {}) | ||
| for tf in tag_filters: | ||
| key, value = _parse_label_filter(tf) | ||
| if value is None: | ||
| if key not in tags: | ||
| return False | ||
| else: | ||
| if tags.get(key) != value: | ||
| return False | ||
| return True | ||
|
|
||
|
|
||
| def _format_labels(obj: Union[OrganizationNode, Folder, Project]) -> str: | ||
| """Format labels as comma-separated key=value string.""" | ||
| labels = getattr(obj, "labels", {}) | ||
| if not labels: | ||
| return "" | ||
| return ", ".join(f"{k}={v}" for k, v in sorted(labels.items())) | ||
|
|
||
|
|
||
| def _format_tags(obj: Union[OrganizationNode, Folder, Project]) -> str: | ||
| """Format tags as comma-separated key=value string.""" | ||
| tags = getattr(obj, "tags", {}) | ||
| if not tags: | ||
| return "" | ||
| return ", ".join(f"{k}={v}" for k, v in sorted(tags.items())) |
Contributor
There was a problem hiding this comment.
There's some code duplication in the newly added helper functions: _matches_labels and _matches_tags are nearly identical, as are _format_labels and _format_tags. To improve maintainability, consider creating generic helper functions that can handle both labels and tags by taking the attribute name as a parameter. This aligns with the practice of refactoring logic into explicit helper functions for better clarity.
References
- When logic is repetitive or complex, refactor it into a more explicit helper function to improve maintainability and readability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/gcpath/serializers.py (1)
72-77:⚠️ Potential issue | 🟠 MajorTree serialization drops folder labels/tags.
Folder nodes produced via
_node_to_dictnever includelabels/tags, soserialize_treeomits metadata for folders even when present.💡 Proposed fix
def _node_to_dict(node: Union[OrganizationNode, Folder]) -> Tuple[str, Dict[str, Any]]: @@ - return node.name, { + d: Dict[str, Any] = { "path": node.path, "resource_name": node.name, "display_name": node.display_name, "type": "folder", } + if node.labels: + d["labels"] = node.labels + if node.tags: + d["tags"] = node.tags + return node.name, d🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gcpath/serializers.py` around lines 72 - 77, The folder branch in _node_to_dict returns a dict with "path", "resource_name", "display_name", and "type" but drops folder metadata like labels/tags, causing serialize_tree to omit them; update _node_to_dict (the folder-returning branch) to include the node's labels and/or tags (e.g., "labels": node.labels and/or "tags": node.tags) in the returned mapping so serialize_tree will preserve folder metadata, and ensure keys match what serialize_tree expects.src/gcpath/cli.py (3)
1355-1394:⚠️ Potential issue | 🟠 Major
ancestors' new metadata flags are no-ops.
show_labelsandshow_tagsare parsed, but this command still resolves only(resource_name, display_name, type)and never loads or renders metadata in text/JSON/YAML output. Either wire the flags into a metadata-aware lookup path or remove them for now.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gcpath/cli.py` around lines 1355 - 1394, The new show_labels and show_tags options are parsed but unused; update the ancestors command to pass these flags into the ancestry lookup and output logic: call Hierarchy.resolve_ancestry_chain(resource_name, include_labels=show_labels, include_tags=show_tags) (or add equivalent params) so the resolver returns metadata alongside (resource_name, display_name, type), then update serialize_ancestors and the text/table rendering in ancestors to include label/tag fields when present (add columns and include metadata in JSON/YAML dump via _get_dumper), or if you prefer not to support metadata yet, remove the show_labels and show_tags options from the ancestors function signature to avoid no-ops.
441-484:⚠️ Potential issue | 🟠 MajorDon't reuse lean caches for metadata-driven commands.
_try_read_cache()is still keyed only by scope, so a cache populated by plainls/tree/findis reused for later--show-labels,--show-tags,--label, or--tagruns. Those commands then render/filter against empty dicts and silently miss matches.Minimal safe fix
- if is_cacheable and not force_refresh: + requires_fresh_metadata = include_labels or include_tags + if is_cacheable and not force_refresh and not requires_fresh_metadata: cached = _try_read_cache(cache_scope, filter_orgs) if cached is not None: return cached🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gcpath/cli.py` around lines 441 - 484, The cached hierarchy is being reused for metadata-driven commands because _try_read_cache is only keyed by scope; to fix, in _load_hierarchy treat requests that need metadata as non-cacheable: when include_labels or include_tags is True (and any other metadata-affecting flags/commands you support, e.g., label/tag mutators), set is_cacheable = False (or clear cache_scope) so the function skips reading/writing the lean cache; adjust the is_cacheable calculation near the is_cacheable variable and ensure write_cache is not invoked for those metadata requests (references: function _load_hierarchy, variables is_cacheable, cache_scope, effective_recursive, calls to _try_read_cache and write_cache).
902-1002:⚠️ Potential issue | 🟠 Major
tree's new label/tag options are only partially wired.The flags currently trigger metadata loading and descendant formatting, but
label_filters/tag_filtersnever prune the tree, sogcpath tree --label ...still renders the full hierarchy. A scoped root folder is also rendered fromnode.path, so--show-labels/--show-tagswon't surface metadata on that root node either.
🧹 Nitpick comments (3)
tests/test_serializers.py (1)
226-257: Add a tree-serialization metadata regression test.This class validates
serialize_resource, but not folder metadata inserialize_tree_node/serialize_tree. A targeted tree test would catch metadata loss on folder nodes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_serializers.py` around lines 226 - 257, The test suite covers serialize_resource but misses verifying folder metadata when serializing entire trees; add a new test (e.g., in tests/test_serializers.py alongside TestSerializeResourceWithLabelsAndTags) that constructs a hierarchy with folder labels/tags, calls serialize_tree_node or serialize_tree on the root, and asserts that folder nodes in the returned tree retain their "labels" and "tags" (and that empty ones are omitted) to catch regressions in serialize_tree_node/serialize_tree handling of folder metadata; reference serialize_tree_node, serialize_tree, and serialize_resource to locate relevant serialization logic and mirror the existing assertions for folders and projects.tests/test_parsers.py (1)
333-351: Add a wrapped-value case forextract_labels.Please add a case where label map values are wrapper objects (e.g.,
{"env": {"v": "prod"}}) so parser behavior is validated against MapComposite-like value shapes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_parsers.py` around lines 333 - 351, Add a new unit test for extract_labels that covers wrapped-map values (MapComposite-like shapes): create a labels_col such as {"v": {"env": {"v": "prod"}, "team": {"v": "infra"}}}, call extract_labels(labels_col) and assert it returns {"env": "prod", "team": "infra"}; name it something like test_extract_labels_wrapped_values and place it alongside the other extract_labels tests to validate unwrapping behavior in the extract_labels function.tests/test_cli.py (1)
1251-1310: AssertHierarchy.loadopt-in flags in CLI tests.The new tests verify output, but they don’t check
include_labels/include_tagskwargs onHierarchy.load. Adding those assertions will guard the cost-sensitive opt-in behavior.💡 Example assertion pattern
result = runner.invoke(app, ["ls", "--show-labels", "-l"]) assert result.exit_code == 0 +mock_load.assert_called_once() +assert mock_load.call_args.kwargs.get("include_labels") is Trueresult = runner.invoke(app, ["ls", "--show-tags", "-l", "-R"]) assert result.exit_code == 0 +mock_load.assert_called_once() +assert mock_load.call_args.kwargs.get("include_tags") is True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli.py` around lines 1251 - 1310, Update the four CLI tests to assert that Hierarchy.load is called with the correct opt-in flags: in test_ls_show_labels assert mock_load.called and mock_load.assert_called_with(..., include_labels=True) (or at least check include_labels=True in the call kwargs) since runner.invoke(app, ["ls", "--show-labels", "-l"]) should opt in; in test_ls_label_filter and test_ls_label_filter_key_only verify Hierarchy.load was called with include_labels=True because they invoke ["ls", "-R", "--label", ...]; and in test_ls_show_tags_long assert mock_load was called with include_tags=True for runner.invoke(app, ["ls", "--show-tags", "-l", "-R"]); for tests that do not request labels/tags ensure include_labels/include_tags are False (or absent) accordingly by checking the call kwargs on mock_load to guard the opt-in behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gcpath/core.py`:
- Around line 226-230: The tag-loading block currently always calls
load_tags_asset(org_node.organization.name) which queries TagBindings at the org
root; change it to use the same scope used for the resource load so
folder-scoped runs only fetch tags within that scope. Specifically, in the
include_tags && !via_resource_manager branch replace the org root argument with
the actual scope identifier used elsewhere (e.g., the folder or project resource
name stored on org_node or the top-level scope variable) so load_tags_asset(...)
is invoked with the requested scope and then call apply_tags(hierarchy,
tags_map) as before.
---
Outside diff comments:
In `@src/gcpath/cli.py`:
- Around line 1355-1394: The new show_labels and show_tags options are parsed
but unused; update the ancestors command to pass these flags into the ancestry
lookup and output logic: call Hierarchy.resolve_ancestry_chain(resource_name,
include_labels=show_labels, include_tags=show_tags) (or add equivalent params)
so the resolver returns metadata alongside (resource_name, display_name, type),
then update serialize_ancestors and the text/table rendering in ancestors to
include label/tag fields when present (add columns and include metadata in
JSON/YAML dump via _get_dumper), or if you prefer not to support metadata yet,
remove the show_labels and show_tags options from the ancestors function
signature to avoid no-ops.
- Around line 441-484: The cached hierarchy is being reused for metadata-driven
commands because _try_read_cache is only keyed by scope; to fix, in
_load_hierarchy treat requests that need metadata as non-cacheable: when
include_labels or include_tags is True (and any other metadata-affecting
flags/commands you support, e.g., label/tag mutators), set is_cacheable = False
(or clear cache_scope) so the function skips reading/writing the lean cache;
adjust the is_cacheable calculation near the is_cacheable variable and ensure
write_cache is not invoked for those metadata requests (references: function
_load_hierarchy, variables is_cacheable, cache_scope, effective_recursive, calls
to _try_read_cache and write_cache).
In `@src/gcpath/serializers.py`:
- Around line 72-77: The folder branch in _node_to_dict returns a dict with
"path", "resource_name", "display_name", and "type" but drops folder metadata
like labels/tags, causing serialize_tree to omit them; update _node_to_dict (the
folder-returning branch) to include the node's labels and/or tags (e.g.,
"labels": node.labels and/or "tags": node.tags) in the returned mapping so
serialize_tree will preserve folder metadata, and ensure keys match what
serialize_tree expects.
---
Nitpick comments:
In `@tests/test_cli.py`:
- Around line 1251-1310: Update the four CLI tests to assert that Hierarchy.load
is called with the correct opt-in flags: in test_ls_show_labels assert
mock_load.called and mock_load.assert_called_with(..., include_labels=True) (or
at least check include_labels=True in the call kwargs) since runner.invoke(app,
["ls", "--show-labels", "-l"]) should opt in; in test_ls_label_filter and
test_ls_label_filter_key_only verify Hierarchy.load was called with
include_labels=True because they invoke ["ls", "-R", "--label", ...]; and in
test_ls_show_tags_long assert mock_load was called with include_tags=True for
runner.invoke(app, ["ls", "--show-tags", "-l", "-R"]); for tests that do not
request labels/tags ensure include_labels/include_tags are False (or absent)
accordingly by checking the call kwargs on mock_load to guard the opt-in
behavior.
In `@tests/test_parsers.py`:
- Around line 333-351: Add a new unit test for extract_labels that covers
wrapped-map values (MapComposite-like shapes): create a labels_col such as {"v":
{"env": {"v": "prod"}, "team": {"v": "infra"}}}, call extract_labels(labels_col)
and assert it returns {"env": "prod", "team": "infra"}; name it something like
test_extract_labels_wrapped_values and place it alongside the other
extract_labels tests to validate unwrapping behavior in the extract_labels
function.
In `@tests/test_serializers.py`:
- Around line 226-257: The test suite covers serialize_resource but misses
verifying folder metadata when serializing entire trees; add a new test (e.g.,
in tests/test_serializers.py alongside TestSerializeResourceWithLabelsAndTags)
that constructs a hierarchy with folder labels/tags, calls serialize_tree_node
or serialize_tree on the root, and asserts that folder nodes in the returned
tree retain their "labels" and "tags" (and that empty ones are omitted) to catch
regressions in serialize_tree_node/serialize_tree handling of folder metadata;
reference serialize_tree_node, serialize_tree, and serialize_resource to locate
relevant serialization logic and mirror the existing assertions for folders and
projects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc820f85-fe5d-4e1b-a3c2-7cd35a0fd478
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
src/gcpath/cache.pysrc/gcpath/cli.pysrc/gcpath/core.pysrc/gcpath/formatters.pysrc/gcpath/loaders.pysrc/gcpath/parsers.pysrc/gcpath/serializers.pytests/fixtures/sample_cache_v2.jsontests/test_cache.pytests/test_cli.pytests/test_formatters.pytests/test_loaders.pytests/test_parsers.pytests/test_serializers.py
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/gcpath/cli.py (2)
436-455:⚠️ Potential issue | 🟠 MajorDon’t reuse metadata-incomplete cache entries.
The cache lookup here ignores
include_labelsandinclude_tags. If the cache was populated by a plain load, later--show-labels,--show-tags,--label, or--tagcommands will read empty metadata from cache and produce wrong output until-Fis used. Gate cache reuse on metadata completeness, or persisthas_labels/has_tagsin cache metadata and validate it here.🩹 Safe stopgap
- if is_cacheable and not force_refresh: + if is_cacheable and not force_refresh and not (include_labels or include_tags): cached = _try_read_cache(cache_scope, filter_orgs) if cached is not None: return cached🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gcpath/cli.py` around lines 436 - 455, The cache lookup currently reuses entries regardless of requested metadata flags (include_labels/include_tags), causing incomplete metadata to be returned; update the cache validation in the _try_read_cache / cache reuse logic to verify that the cached entry actually contains the requested metadata (either by persisting and checking cache metadata fields like has_labels and has_tags when write_cache is called, or by gating reuse based on include_labels/include_tags), and only return cached when those metadata completeness flags match the current request; if they don’t match, treat as cache miss (fall through to Hierarchy.load and then write_cache with the appropriate has_labels/has_tags metadata).
815-840:⚠️ Potential issue | 🟠 Major
ls --show-labels/--show-tagsis a no-op in default text mode.The new metadata columns are only added inside the
if long:branch. In the default path,ls --show-labelsstill prints bare paths, so the advertised flags look broken unless callers also know to add--long. Either switch to the table automatically when metadata display is requested, or append metadata to the plain-text line format.💡 Minimal local fix
- if long: + if long or show_labels or show_tags: table = Table( show_header=True, header_style="bold magenta", box=None, padding=(0, 1) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gcpath/cli.py` around lines 815 - 840, The flags show_labels/show_tags are ignored when long is False because the plain-text branch prints only paths; change the else branch to iterate for path, obj in items (not path, _) and append metadata using _format_metadata(obj, "labels") and/or _format_metadata(obj, "tags") to the printed line when show_labels or show_tags is set (or alternatively switch to using the table output when either flag is true); update the loop in the non-long branch to build a single string per item that includes the path plus the requested metadata fields and print that.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gcpath/cli.py`:
- Around line 895-902: The tree command currently only toggles
include_labels/include_tags but never prunes the hierarchy by
label_filters/tag_filters, so implement a pruning step that filters the built
hierarchy to only nodes matching the provided label_filters/tag_filters plus any
ancestors required to reach them; add a helper (e.g. prune_tree_by_filters) and
call it in the tree path after _prepare_hierarchy_command and before
build_tree_view/serialization (or modify build_tree_view to accept
label_filters/tag_filters and do the pruning internally), using the existing
label_filters and tag_filters variables and preserving
include_labels/include_tags for display.
In `@src/gcpath/core.py`:
- Around line 226-233: The code currently skips tag loading when
via_resource_manager is true, causing --show-tags/--tag to silently return no
bindings; update the logic in the tag-loading blocks (the if guard that checks
include_tags and not via_resource_manager) to either (A) raise/fail fast with a
clear error when include_tags is requested but
via_resource_manager/--no-use-asset-api prevents tag queries, or (B) allow tag
binding queries to run even when via_resource_manager is true by invoking
load_tags_asset(tag_scope) and apply_tags(hierarchy, tags_map) for the same
tag_scopes; make the same change for the other occurrence referenced around
lines 388-390 so tag behavior is consistent across both code paths.
In `@src/gcpath/formatters.py`:
- Around line 171-186: _format_metadata_suffix builds Rich markup fragments in
parts (e.g., parts entries already contain [dim]...[/dim]) but currently wraps
the whole suffix in literal brackets ("[" + ... + "]"), causing Rich to escape
and render them as literal text; remove the outer brackets so the function
returns a leading space plus the joined parts (i.e., " " + " | ".join(parts)) so
Rich can parse the inner [dim] markup correctly; update the return expression in
_format_metadata_suffix and keep using show_labels/show_tags, parts, label_str
and tag_str as before.
---
Outside diff comments:
In `@src/gcpath/cli.py`:
- Around line 436-455: The cache lookup currently reuses entries regardless of
requested metadata flags (include_labels/include_tags), causing incomplete
metadata to be returned; update the cache validation in the _try_read_cache /
cache reuse logic to verify that the cached entry actually contains the
requested metadata (either by persisting and checking cache metadata fields like
has_labels and has_tags when write_cache is called, or by gating reuse based on
include_labels/include_tags), and only return cached when those metadata
completeness flags match the current request; if they don’t match, treat as
cache miss (fall through to Hierarchy.load and then write_cache with the
appropriate has_labels/has_tags metadata).
- Around line 815-840: The flags show_labels/show_tags are ignored when long is
False because the plain-text branch prints only paths; change the else branch to
iterate for path, obj in items (not path, _) and append metadata using
_format_metadata(obj, "labels") and/or _format_metadata(obj, "tags") to the
printed line when show_labels or show_tags is set (or alternatively switch to
using the table output when either flag is true); update the loop in the
non-long branch to build a single string per item that includes the path plus
the requested metadata fields and print that.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0ebbe4a-c8cb-41d4-9d37-909e0855b975
📒 Files selected for processing (6)
src/gcpath/cache.pysrc/gcpath/cli.pysrc/gcpath/core.pysrc/gcpath/formatters.pysrc/gcpath/loaders.pysrc/gcpath/parsers.py
✅ Files skipped from review due to trivial changes (1)
- src/gcpath/cache.py
Comment on lines
895
to
902
| # Implicitly enable label/tag fetching when filters are specified | ||
| include_labels = show_labels or bool(label_filters) | ||
| include_tags = show_tags or bool(tag_filters) | ||
|
|
||
| hctx = _prepare_hierarchy_command( | ||
| ctx, "tree", resource, level, yes, force_refresh | ||
| ctx, "tree", resource, level, yes, force_refresh, | ||
| include_labels=include_labels, include_tags=include_tags, | ||
| ) |
There was a problem hiding this comment.
Apply --label / --tag to the tree output.
These flags currently only flip include_labels / include_tags; neither the serializer path nor build_tree_view() ever filters by label_filters / tag_filters. gcpath tree --label ... still renders the full hierarchy. The tree path needs a pruning step that keeps matching nodes plus the ancestors needed to reach them.
Also applies to: 914-976
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gcpath/cli.py` around lines 895 - 902, The tree command currently only
toggles include_labels/include_tags but never prunes the hierarchy by
label_filters/tag_filters, so implement a pruning step that filters the built
hierarchy to only nodes matching the provided label_filters/tag_filters plus any
ancestors required to reach them; add a helper (e.g. prune_tree_by_filters) and
call it in the tree path after _prepare_hierarchy_command and before
build_tree_view/serialization (or modify build_tree_view to accept
label_filters/tag_filters and do the pruning internally), using the existing
label_filters and tag_filters variables and preserving
include_labels/include_tags for display.
Comment on lines
+226
to
+233
| # Load tags if requested (separate Asset API query) | ||
| if include_tags and not via_resource_manager: | ||
| tag_scopes = [scope_resource] if scope_resource else [ | ||
| org_node.organization.name for org_node in org_nodes | ||
| ] | ||
| for tag_scope in tag_scopes: | ||
| tags_map = load_tags_asset(tag_scope) | ||
| apply_tags(hierarchy, tags_map) |
There was a problem hiding this comment.
Tag requests are silently dropped in Resource Manager mode.
Both tag-loading paths still guard on not via_resource_manager, but the CLI now exposes --show-tags / --tag regardless of backend. In practice, --no-use-asset-api will load no tag bindings and can return empty or incorrectly filtered results. Either fail fast on that flag combination, or document and allow the separate TagBinding query even when folders/projects come from Resource Manager.
Also applies to: 388-390
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gcpath/core.py` around lines 226 - 233, The code currently skips tag
loading when via_resource_manager is true, causing --show-tags/--tag to silently
return no bindings; update the logic in the tag-loading blocks (the if guard
that checks include_tags and not via_resource_manager) to either (A) raise/fail
fast with a clear error when include_tags is requested but
via_resource_manager/--no-use-asset-api prevents tag queries, or (B) allow tag
binding queries to run even when via_resource_manager is true by invoking
load_tags_asset(tag_scope) and apply_tags(hierarchy, tags_map) for the same
tag_scopes; make the same change for the other occurrence referenced around
lines 388-390 so tag behavior is consistent across both code paths.
Comment on lines
+171
to
+186
| def _format_metadata_suffix( | ||
| item: Union[Folder, Project], | ||
| show_labels: bool = False, | ||
| show_tags: bool = False, | ||
| ) -> str: | ||
| """Build a Rich markup suffix for labels and tags.""" | ||
| parts = [] | ||
| if show_labels and hasattr(item, "labels") and item.labels: | ||
| label_str = ", ".join(f"{k}={v}" for k, v in sorted(item.labels.items())) | ||
| parts.append(f"[dim]labels: {label_str}[/dim]") | ||
| if show_tags and hasattr(item, "tags") and item.tags: | ||
| tag_str = ", ".join(f"{k}={v}" for k, v in sorted(item.tags.items())) | ||
| parts.append(f"[dim]tags: {tag_str}[/dim]") | ||
| if not parts: | ||
| return "" | ||
| return " [" + " | ".join(parts) + "]" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import subprocess
import sys
try:
from rich.text import Text
except ImportError:
subprocess.check_call([sys.executable, "-m", "pip", "install", "rich"])
from rich.text import Text
samples = [
" [dim]labels: env=prod[/dim]",
" [[dim]labels: env=prod[/dim]]",
]
for sample in samples:
try:
text = Text.from_markup(sample)
print("OK ", repr(sample), "->", repr(text.plain))
except Exception as exc:
print("ERR", repr(sample), type(exc).__name__, exc)
PYRepository: tardigrde/gcpath
Length of output: 179
🏁 Script executed:
cat -n src/gcpath/formatters.py | sed -n '171,186p'Repository: tardigrde/gcpath
Length of output: 845
Don't wrap Rich markup fragments in literal [].
_format_metadata_suffix() returns strings like " [[dim]labels: env=prod[/dim]]". In Rich, the leading [[ escapes the bracket and produces literal output [labels: env=prod] without the dim formatting applied. Remove the outer brackets to properly parse the markup.
💡 Minimal fix
- return " [" + " | ".join(parts) + "]"
+ return " " + " | ".join(parts)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gcpath/formatters.py` around lines 171 - 186, _format_metadata_suffix
builds Rich markup fragments in parts (e.g., parts entries already contain
[dim]...[/dim]) but currently wraps the whole suffix in literal brackets ("[" +
... + "]"), causing Rich to escape and render them as literal text; remove the
outer brackets so the function returns a leading space plus the joined parts
(i.e., " " + " | ".join(parts)) so Rich can parse the inner [dim] markup
correctly; update the return expression in _format_metadata_suffix and keep
using show_labels/show_tags, parts, label_str and tag_str as before.
- Remove unimplemented --show-labels/--show-tags from ancestors command - Deduplicate _matches_labels/_matches_tags into generic _matches_metadata - Deduplicate _format_labels/_format_tags into generic _format_metadata - Use scope_resource for tag lookups instead of always querying org root - Replace duplicated "organizations/" literal with _RESOURCE_PREFIX_ORGS constant - Sanitize user-controlled data from cache log message - Fix single-iteration loop in build_folder_ancestors (parsers.py) - Reduce cognitive complexity across cli.py, core.py, formatters.py, loaders.py, and parsers.py by extracting helper functions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add opt-in support for GCP resource labels (key-value pairs) and resource
tags (Tag Manager bindings) across CLI commands. Labels are fetched via an
additional SQL column in Asset API queries (cheap). Tags require a separate
Asset API query against TagBinding resources (expensive). Both are only
fetched when explicitly requested via CLI flags.
New CLI options on ls, tree, find, ancestors:
Changes across the stack:
https://claude.ai/code/session_01HugtU9fbaL97tbb7zaqNPL
Summary by CodeRabbit