feat: strip hidden labels from exporter display output#892
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (23)
🚧 Files skipped from review as they are similar to previous changes (21)
📝 WalkthroughWalkthroughExporter label keys can now be hidden through controller configuration and optionally returned with ChangesHidden exporter label visibility
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ClientConfig
participant ListExportersRPC
participant ClientService
CLI->>ClientConfig: request exporters with show_hidden
ClientConfig->>ListExportersRPC: send show_hidden
ListExportersRPC->>ClientService: deliver exporter request
ClientService-->>ListExportersRPC: filter configured labels unless show_hidden
ListExportersRPC-->>ClientConfig: return exporter labels
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/client/grpc.py (1)
345-346: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd type hints to
_dump_exporter.For improved developer ergonomics and static analysis, consider adding type annotations for
exporterandexclude_fields.💡 Proposed refactor
- def _dump_exporter(self, exporter, exclude_fields) -> dict: + def _dump_exporter(self, exporter: Exporter, exclude_fields: set[str]) -> dict: return exporter.model_dump(mode="json", exclude=exclude_fields)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter/jumpstarter/client/grpc.py` around lines 345 - 346, Add explicit type annotations to the `_dump_exporter` method for both `exporter` and `exclude_fields`, while retaining its existing `dict` return annotation. Use the concrete exporter model and exclusion-field types already established in the surrounding client code or imported definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/client/grpc.py`:
- Around line 345-346: Add explicit type annotations to the `_dump_exporter`
method for both `exporter` and `exclude_fields`, while retaining its existing
`dict` return annotation. Use the concrete exporter model and exclusion-field
types already established in the surrounding client code or imported
definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e67ed262-0555-41be-ad3a-19daee17cad1
📒 Files selected for processing (6)
python/packages/jumpstarter-cli/jumpstarter_cli/get.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/server.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/tools/leases.pypython/packages/jumpstarter/jumpstarter/client/grpc.pypython/packages/jumpstarter/jumpstarter/client/grpc_test.pypython/packages/jumpstarter/jumpstarter/config/client.py
|
@mangelajo i am not set on the prefix, we could also use |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
python/packages/jumpstarter-cli/jumpstarter_cli/get_test.py (1)
224-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the explicit
show_hidden=Truepath.This test only verifies the default/hidden-label-filtering path. Add a second case or parameterize the test with
show_hidden=True; otherwise a regression that always forwardsFalsewould still pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-cli/jumpstarter_cli/get_test.py` around lines 224 - 231, Extend the test around get_exporters.callback.__wrapped__.__wrapped__ to cover show_hidden=True, either by adding a separate case or parameterizing the existing case. Assert that config.list_exporters receives show_hidden=True for that invocation, while preserving the existing default-path assertion.python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py (1)
191-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the hidden-label prefix single-sourced.
The docstring duplicates
hidden.jumpstarter.dev/, while filtering uses the shared prefix constant. Since the prefix is not finalized, a future change could make the MCP documentation incorrect. Reuse a shared constant or centralize the user-facing description.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py` at line 191, Update the documentation for the show_hidden option near the server argument definition to derive or reuse the existing shared hidden-label prefix constant instead of hardcoding hidden.jumpstarter.dev/. Keep the user-facing description accurate if that constant changes, without duplicating the prefix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/get_test.py`:
- Around line 224-231: Extend the test around
get_exporters.callback.__wrapped__.__wrapped__ to cover show_hidden=True, either
by adding a separate case or parameterizing the existing case. Assert that
config.list_exporters receives show_hidden=True for that invocation, while
preserving the existing default-path assertion.
In `@python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py`:
- Line 191: Update the documentation for the show_hidden option near the server
argument definition to derive or reuse the existing shared hidden-label prefix
constant instead of hardcoding hidden.jumpstarter.dev/. Keep the user-facing
description accurate if that constant changes, without duplicating the prefix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ed1c3e9-3125-4c1a-a884-10e9b60a3243
📒 Files selected for processing (7)
python/packages/jumpstarter-cli/jumpstarter_cli/get.pypython/packages/jumpstarter-cli/jumpstarter_cli/get_test.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/server.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/tools/leases.pypython/packages/jumpstarter/jumpstarter/client/grpc.pypython/packages/jumpstarter/jumpstarter/client/grpc_test.pypython/packages/jumpstarter/jumpstarter/config/client.py
🚧 Files skipped from review as they are similar to previous changes (3)
- python/packages/jumpstarter-cli/jumpstarter_cli/get.py
- python/packages/jumpstarter/jumpstarter/client/grpc_test.py
- python/packages/jumpstarter/jumpstarter/client/grpc.py
"_" would be invalid for k8s I believe I was thinking, what if we added an annotation to exporters to list hidden labels, would be very helpful to deprecate labels. May be even later a deprecated annotation to warn users when they are using a phased out label. i.e. annotations: (but the deprecated is another feature tb...) Or a better option could be (and that could make more sense) to provide a list in the controller config + operator: labels: WDYT? I know you're trying to avoid controller update in this case, but we can backport this and bring it to the lab quickly |
it is legal https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set
yeah i was trying to avoid controller changes for something like this, i'll have a look |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controller/deploy/operator/api/v1alpha1/jumpstarter_types.go`:
- Around line 187-190: Move the HiddenLabels field and its associated comments
from the top-level specification into ControllerConfig, preserving its JSON
name, optional behavior, and HiddenLabelsConfig type. Update any references to
read the setting through ControllerConfig while leaving global settings such as
BaseDomain and Authentication unchanged.
In `@python/packages/jumpstarter/jumpstarter/client/grpc.py`:
- Around line 339-341: The _dump_exporter helper hardcodes JSON serialization,
causing model_dump to lose native Python types. In
python/packages/jumpstarter/jumpstarter/client/grpc.py lines 339-341, update
_dump_exporter to accept and forward **kwargs to exporter.model_dump; in lines
357-357, pass mode="json" explicitly at the model_dump_json call site so JSON
output remains serializable.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e15a8ae9-c0ac-450a-9b29-1bc512668776
⛔ Files ignored due to path filters (2)
controller/internal/protocol/jumpstarter/client/v1/client.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/client/v1/client.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (22)
controller/buf.gen.yamlcontroller/cmd/main.gocontroller/deploy/operator/api/v1alpha1/jumpstarter_types.gocontroller/deploy/operator/api/v1alpha1/zz_generated.deepcopy.gocontroller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yamlcontroller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.gocontroller/internal/config/config.gocontroller/internal/config/types.gocontroller/internal/config/types_test.gocontroller/internal/service/client/v1/client_service.gocontroller/internal/service/client/v1/client_service_test.gocontroller/internal/service/controller_service.goprotocol/proto/jumpstarter/client/v1/client.protopython/packages/jumpstarter-cli/jumpstarter_cli/get.pypython/packages/jumpstarter-cli/jumpstarter_cli/get_test.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/server.pypython/packages/jumpstarter-mcp/jumpstarter_mcp/tools/leases.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pypython/packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.pyipython/packages/jumpstarter/jumpstarter/client/grpc.pypython/packages/jumpstarter/jumpstarter/client/grpc_test.pypython/packages/jumpstarter/jumpstarter/config/client.py
🚧 Files skipped from review as they are similar to previous changes (4)
- python/packages/jumpstarter/jumpstarter/config/client.py
- python/packages/jumpstarter-mcp/jumpstarter_mcp/tools/leases.py
- python/packages/jumpstarter-mcp/jumpstarter_mcp/server.py
- python/packages/jumpstarter-cli/jumpstarter_cli/get.py
| // Hidden labels configuration for hiding specific label keys from exporter listings. | ||
| // +optional | ||
| HiddenLabels HiddenLabelsConfig `json:"hiddenLabels,omitempty"` | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move HiddenLabels into ControllerConfig.
As per coding guidelines: "Place controller-specific settings (image, replicas, grpc endpoints) within a controller-level configuration, not at the top-level spec".
HiddenLabels governs how the controller filters gRPC API responses, making it a controller-specific setting rather than a global one like BaseDomain or Authentication. Please move it into ControllerConfig.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/deploy/operator/api/v1alpha1/jumpstarter_types.go` around lines
187 - 190, Move the HiddenLabels field and its associated comments from the
top-level specification into ControllerConfig, preserving its JSON name,
optional behavior, and HiddenLabelsConfig type. Update any references to read
the setting through ControllerConfig while leaving global settings such as
BaseDomain and Authentication unchanged.
Source: Coding guidelines
Add hiddenLabels.keys to the controller ConfigMap and operator CRD. The server strips matching label keys from ListExporters/GetExporter responses. Clients pass show_hidden in the proto request to bypass filtering (jmp get exporters --show-hidden). This is useful when there is data we need but don't want to pollute the label output too much Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
| (google.api.resource_reference) = {type: "jumpstarter.dev/Exporter"} | ||
| ]; | ||
| // When true, include labels hidden by controller configuration. | ||
| bool show_hidden = 2 [(google.api.field_behavior) = OPTIONAL]; |
There was a problem hiding this comment.
| bool show_hidden = 2 [(google.api.field_behavior) = OPTIONAL]; | |
| bool show_hidden_labels = 2 [(google.api.field_behavior) = OPTIONAL]; |
Otherwise could be mistaken by show hidden "exporters" which would be my first understanding outside of the context of this pr.
| // Filter expression for the results. | ||
| string filter = 4 [(google.api.field_behavior) = OPTIONAL]; | ||
| // When true, include labels hidden by controller configuration. | ||
| bool show_hidden = 5 [(google.api.field_behavior) = OPTIONAL]; |
There was a problem hiding this comment.
| bool show_hidden = 5 [(google.api.field_behavior) = OPTIONAL]; | |
| bool show_hidden_labels = 5 [(google.api.field_behavior) = OPTIONAL]; |
| help="Include disabled exporters in the listing", | ||
| ) | ||
| @click.option( | ||
| "--show-hidden", |
There was a problem hiding this comment.
for the same reasons:
| "--show-hidden", | |
| "--show-hidden-labels", |
it's also a parameter that wouldn't be used so often, so it's ok to make it a little bit longer IMO.
| @handle_exceptions_with_reauthentication(relogin_client) | ||
| def get_exporters(config, selector: str | None, output: OutputType, with_options: list[str], allow_disabled: bool): | ||
| def get_exporters( | ||
| config, selector: str | None, output: OutputType, with_options: list[str], allow_disabled: bool, show_hidden: bool |
There was a problem hiding this comment.
| config, selector: str | None, output: OutputType, with_options: list[str], allow_disabled: bool, show_hidden: bool | |
| config, selector: str | None, output: OutputType, with_options: list[str], allow_disabled: bool, show_hidden_labels: bool |
2 participants
Labels prefixed with hidden.jumpstarter.dev/ are excluded from jmp get exporters. Pass --show-hidden
to reveal them. Filtering happens at the data boundary in list_exporters() so all display paths get consistent behavior.
This is useful when there is data we need but don't want to pollute the label output too much