feat: add tags field to lease, to allow user set metadata#622
Draft
feat: add tags field to lease, to allow user set metadata#622
Conversation
Add map<string, string> tags field to the Lease protobuf message and Tags map[string]string to the K8s LeaseSpec CRD. Fix protobuf Python import paths to use relative imports. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (1)
controller/api/v1alpha1/lease_helpers_test.go (1)
380-475: Add coverage for qualified tag keys.The validation tests reject reserved prefixes, but do not cover arbitrary qualified tag keys like
example.com/team. When such keys are stored as labels undermetadata.jumpstarter.dev/example.com/team, they violate Kubernetes qualified name format (which allows at most one slash). Although the current implementation correctly rejects these viaIsQualifiedName()validation, this behavior lacks explicit test coverage.Suggested test
It("should reject reserved prefix metadata.jumpstarter.dev/", func() { tags := map[string]string{ "metadata.jumpstarter.dev/team": "value", } err := ValidateLeaseTags(tags, 10) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("reserved prefix")) }) + + It("should reject qualified tag keys", func() { + tags := map[string]string{ + "example.com/team": "devops", + } + err := ValidateLeaseTags(tags, 10) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("tag key")) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/api/v1alpha1/lease_helpers_test.go` around lines 380 - 475, Add a test to cover qualified tag keys by calling ValidateLeaseTags with a tag key like "example.com/team" (e.g. tags := map[string]string{"example.com/team":"value"}) and assert it returns an error; specifically Expect(ValidateLeaseTags(tags, 10)).To(HaveOccurred()) and check the error message contains the label/qualified-name validation text (e.g. "not a valid label key" or "qualified name") so the behavior enforced by ValidateLeaseTags is explicitly tested.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/api/v1alpha1/lease_helpers.go`:
- Around line 220-227: When copying selector.MatchLabels into metaLabels, skip
keys that are in the reserved tag-metadata namespace (i.e. keys that start with
the LeaseTagMetadataPrefix) so reserved tag labels are not duplicated into
ObjectMeta.Labels; keep the selector unchanged in Spec.Selector for exporter
selection, but only copy non-reserved keys into metaLabels, then continue to add
req.Tags with LeaseTagMetadataPrefix as before (refer to selector.MatchLabels,
metaLabels, LeaseTagMetadataPrefix, req.Tags and Spec.Selector to locate the
code).
In `@controller/api/v1alpha1/lease_types.go`:
- Around line 38-41: The CRD currently only limits Tags with MaxProperties=10
but does not enforce immutability or Kubernetes label rules; update the API so
spec.tags cannot be mutated and invalid keys/values are rejected by either
adding CEL validation rules to the generated CRD for the Tags field
(immutability check comparing to oldObject.spec.tags and regex checks for
keys/values) or implement webhook-based validation on the Lease type by adding
ValidateCreate and ValidateUpdate methods that call the existing
ValidateLeaseTags helper and return errors when tags violate immutability or
label rules; refer to the Tags field, the Lease type, and the existing
ValidateLeaseTags function when making these changes.
In `@controller/deploy/operator/api/v1alpha1/jumpstarter_types.go`:
- Around line 188-192: Add a kubebuilder minimum validation tag to
LeasePolicyConfig.MaxTags: update the struct field for MaxTags by adding the
marker `+kubebuilder:validation:Minimum=0` (alongside the existing
`+kubebuilder:default=10`) so negative values are disallowed; then run `make
manifests generate` in controller/deploy/operator to refresh the generated CRD
artifacts.
In `@controller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yaml`:
- Around line 149-156: The CRD documents spec.tags as immutable but lacks a CEL
enforcement; add an x-kubernetes-validations entry that prevents changes to
spec.tags after creation by adding a rule such as: require that either oldSelf
is null (creation) or self.spec.tags == oldSelf.spec.tags (no modification),
with an appropriate message; insert this validation under the schema for tags
(the object with maxProperties: 10 and description) using the
x-kubernetes-validations key and reference spec.tags in the CEL expression to
block kubectl patch/API updates that attempt to change tags.
In
`@controller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml`:
- Around line 1306-1310: The maxTags integer schema lacks bounds and can be set
negative or >10; update the CRD schema for the maxTags property (the maxTags
field in the operator.jumpstarter.dev Jumpstarter CRD) to include "minimum: 1"
and "maximum: 10" so its allowed range is [1,10] and matches the Lease CRD's
spec.tags.maxProperties constraint; keep the existing type/format/default but
add those two constraints to the maxTags schema block.
In
`@controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go`:
- Around line 1148-1151: The LeasePolicy MaxTags value is copied directly from
jumpstarter.Spec.LeasePolicy.MaxTags into cfg.LeasePolicy without guarding
against negatives, which breaks ValidateLeaseTags; add a runtime check in the
controller (before assigning cfg.LeasePolicy) to ensure
jumpstarter.Spec.LeasePolicy.MaxTags is >= 0 (e.g., return a validation error or
coerce to 0) and/or add the CRD marker +kubebuilder:validation:Minimum=0 on the
LeasePolicy.MaxTags field in the API type to prevent negative values at creation
time; update references to cfg.LeasePolicy,
jumpstarter.Spec.LeasePolicy.MaxTags, and ValidateLeaseTags accordingly.
In `@controller/internal/config/config.go`:
- Around line 85-90: LoadConfiguration is returning a zero-valued LeasePolicy
(LeasePolicy{}) which causes LeasePolicy.MaxTags to be 0 and makes CreateLease
-> ValidateLeaseTags(..., int(s.MaxTags)) reject tags; update LoadConfiguration
so both return paths set a sensible default (e.g., MaxTags = 10) when the parsed
LeasePolicy has MaxTags == 0 (or config omits it), i.e., replace returns of
&LeasePolicy{} with &LeasePolicy{MaxTags: 10} (or apply logic to set s.MaxTags =
10 when zero) so CreateLease and ValidateLeaseTags see the intended default.
In `@controller/internal/service/client/v1/client_service.go`:
- Around line 142-160: The tag-filter parsing block that loops over requirements
(labels.Parse -> requirements loop) must reject already-qualified keys before
prefixing: in the loop where you build prefixedKey using
jumpstarterdevv1alpha1.LeaseTagMetadataPrefix + r.Key(), validate r.Key() does
not contain '/' (or otherwise fail IsQualifiedName) and return a
codes.InvalidArgument error if it does; do this check before creating the
labels.NewRequirement so you don't produce multi-slash label keys, and add the
strings import (or the appropriate validator) to the file to support the check.
In `@controller/internal/service/controller_service.go`:
- Around line 1001-1005: The code currently dereferences s.LeasePolicy and
passes *s.LeasePolicy.MaxTags directly into clientsvcv1.NewClientService which
treats a missing or zero MaxTags as 0; instead normalize the lease policy first
(e.g., compute an effective lease policy or copy into a local variable), ensure
a sensible default MaxTags (10) when s.LeasePolicy is nil or MaxTags is 0, and
pass that normalized value into clientsvcv1.NewClientService (referencing
s.LeasePolicy, the local effective lease policy, and
clientsvcv1.NewClientService to locate where to change the call).
In `@docs/internal/jeps/JEP-0012-lease-tags.md`:
- Around line 64-65: Update the wording in JEP-0012 so tag keys are described as
unprefixed label names (single name segment, max 63 characters, DNS-1123
label/name rules) that must not contain a '/' or an explicit prefix; clarify
that the server will automatically prepend the metadata.jumpstarter.dev/ prefix
and users must provide only the unprefixed key (apply the same change to the
similar paragraph covering lines 71–84).
- Around line 33-38: The third user story ("As a developer, I want to annotate
leases") is truncated and missing the benefit clause; update that bullet (the
line starting "As a developer, I want to annotate leases") to include the
outcome/benefit — e.g., complete it as "As a developer, I want to annotate
leases with metadata (owner, ticket, reason), so that I can track intent and
troubleshoot resource usage." Ensure the completed sentence follows the same "As
a ..., I want to ..., so that ..." format used by the other bullets.
- Around line 76-96: The markdown code fences containing CLI examples (the
blocks with "--tag-filter team=devops", "kubectl get leases -l
metadata.jumpstarter.dev/team=devops", and the "jmp create lease..." / "jmp get
leases..." example) are missing language tags and trigger markdownlint; update
each fenced block to use a language of "shell" (i.e., replace ``` with ```shell)
so the CLI examples are properly annotated.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/create_test.py`:
- Line 59: The test creates an unused mock binding by doing with
patch("jumpstarter_cli.create.model_print") as model_print: which triggers Ruff
F841; remove the unused binding by changing the patch context to use
patch("jumpstarter_cli.create.model_print") without "as model_print" (or bind to
an underscore variable like "_model_print" if a binding is required), so the
mock is applied but no unused name remains.
In `@python/packages/jumpstarter/jumpstarter/config/client.py`:
- Line 287: The return call to self._collect_all_leases in the function at this
location is over 120 characters; wrap the arguments across multiple lines so the
call conforms to Ruff's 120-char limit—for example place each keyword argument
(svc, page_size=..., only_active=..., filter=..., tag_filter=...) on its own
line or break after the opening parenthesis and align subsequent lines—ensure
the function name _collect_all_leases and the surrounding return statement
remain unchanged except for formatting.
---
Nitpick comments:
In `@controller/api/v1alpha1/lease_helpers_test.go`:
- Around line 380-475: Add a test to cover qualified tag keys by calling
ValidateLeaseTags with a tag key like "example.com/team" (e.g. tags :=
map[string]string{"example.com/team":"value"}) and assert it returns an error;
specifically Expect(ValidateLeaseTags(tags, 10)).To(HaveOccurred()) and check
the error message contains the label/qualified-name validation text (e.g. "not a
valid label key" or "qualified name") so the behavior enforced by
ValidateLeaseTags is explicitly tested.
🪄 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: 02308427-ec20-4718-9030-a2835de097c6
⛔ Files ignored due to path filters (4)
controller/internal/protocol/jumpstarter/client/v1/client.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/v1/jumpstarter_grpc.pb.gois excluded by!**/*.pb.gocontroller/internal/protocol/jumpstarter/v1/router_grpc.pb.gois excluded by!**/*.pb.gopython/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
controller/api/v1alpha1/lease_helpers.gocontroller/api/v1alpha1/lease_helpers_test.gocontroller/api/v1alpha1/lease_types.gocontroller/api/v1alpha1/zz_generated.deepcopy.gocontroller/cmd/main.gocontroller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_clients.yamlcontroller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporteraccesspolicies.yamlcontroller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporters.yamlcontroller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_leases.yamlcontroller/deploy/operator/api/v1alpha1/jumpstarter_types.gocontroller/deploy/operator/api/v1alpha1/zz_generated.deepcopy.gocontroller/deploy/operator/config/crd/bases/jumpstarter.dev_clients.yamlcontroller/deploy/operator/config/crd/bases/jumpstarter.dev_exporteraccesspolicies.yamlcontroller/deploy/operator/config/crd/bases/jumpstarter.dev_exporters.yamlcontroller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yamlcontroller/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/service/client/v1/client_service.gocontroller/internal/service/controller_service.godocs/internal/jeps/JEP-0012-lease-tags.mdprotocol/proto/jumpstarter/client/v1/client.protopython/packages/jumpstarter-cli/jumpstarter_cli/create.pypython/packages/jumpstarter-cli/jumpstarter_cli/create_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/get.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/client/lease.pypython/packages/jumpstarter/jumpstarter/config/client.py
Comment on lines
+38
to
+41
| // User-defined tags for the lease. Immutable after creation. | ||
| // Maximum 10 entries. Keys and values must conform to Kubernetes label rules. | ||
| // +kubebuilder:validation:MaxProperties=10 | ||
| Tags map[string]string `json:"tags,omitempty"` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether generated CRDs/webhooks currently enforce tag immutability or tag key/value rules.
rg -n -C3 'Tags map\[string\]string|spec\.tags|x-kubernetes-validations|ValidateLeaseTags|ValidateCreate|ValidateUpdate|ValidatingWebhook' controller/api controller/deploy/operator/config/crd controller/deploy/helmRepository: jumpstarter-dev/jumpstarter
Length of output: 10337
🏁 Script executed:
#!/bin/bash
# Search for ValidateCreate/ValidateUpdate methods on Lease type
rg -n -A10 'func \(l \*Lease\).*Validate(Create|Update)' controller/api/v1alpha1/Repository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Get the full tags field definition from the generated CRD
rg -n -A15 'name: tags' controller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yaml | head -30Repository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Search for ValidatingWebhook or MutatingWebhook configurations
find controller/deploy -name '*.yaml' -type f -exec grep -l 'ValidatingWebhook\|MutatingWebhook\|admissionWebhooks' {} \;Repository: jumpstarter-dev/jumpstarter
Length of output: 131
🏁 Script executed:
#!/bin/bash
# Get the complete tags field section from generated CRD
grep -n -A20 '"name": "tags"' controller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yamlRepository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Check what webhook configs are in the kustomization
cat controller/deploy/operator/config/default/kustomization.yamlRepository: jumpstarter-dev/jumpstarter
Length of output: 7658
🏁 Script executed:
#!/bin/bash
# Search for webhook marker annotations or webhook manifests
find controller/deploy/operator/config -name '*webhook*' -type fRepository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Search for tags in the CRD file without assuming specific format
cat controller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yaml | grep -n -i tags | head -20Repository: jumpstarter-dev/jumpstarter
Length of output: 175
🏁 Script executed:
#!/bin/bash
# Find where ValidateLeaseTags is actually called
rg -n 'ValidateLeaseTags' controller/ --type goRepository: jumpstarter-dev/jumpstarter
Length of output: 1704
🏁 Script executed:
#!/bin/bash
# Check the spec schema section of the Lease CRD to see what fields are validated
sed -n '/spec:/,/^[[:space:]]*[a-z]/p' controller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yaml | head -100Repository: jumpstarter-dev/jumpstarter
Length of output: 187
🏁 Script executed:
#!/bin/bash
# Get the complete tags field definition from the CRD
sed -n '149,165p' controller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yamlRepository: jumpstarter-dev/jumpstarter
Length of output: 926
Add CRD-level immutability and label-rule validation for the Tags field.
The Tags field is documented as immutable and label-rule constrained, but the generated CRD enforces only maxProperties: 10. ValidateLeaseTags validates all constraints comprehensively, but it's only called from the gRPC service layer. Direct Kubernetes writes via kubectl or controllers can mutate spec.tags or persist invalid keys/values, bypassing validation and potentially desynchronizing tags from the prefixed metadata labels used for filtering.
Suggested fix
// User-defined tags for the lease. Immutable after creation.
// Maximum 10 entries. Keys and values must conform to Kubernetes label rules.
// +kubebuilder:validation:MaxProperties=10
+ // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="lease tags are immutable"
Tags map[string]string `json:"tags,omitempty"`Add CEL validation rules in the generated CRD to enforce immutability and label-rule conformance for keys and values, or implement ValidateCreate/ValidateUpdate webhook methods on the Lease type.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // User-defined tags for the lease. Immutable after creation. | |
| // Maximum 10 entries. Keys and values must conform to Kubernetes label rules. | |
| // +kubebuilder:validation:MaxProperties=10 | |
| Tags map[string]string `json:"tags,omitempty"` | |
| // User-defined tags for the lease. Immutable after creation. | |
| // Maximum 10 entries. Keys and values must conform to Kubernetes label rules. | |
| // +kubebuilder:validation:MaxProperties=10 | |
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="lease tags are immutable" | |
| Tags map[string]string `json:"tags,omitempty"` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/api/v1alpha1/lease_types.go` around lines 38 - 41, The CRD
currently only limits Tags with MaxProperties=10 but does not enforce
immutability or Kubernetes label rules; update the API so spec.tags cannot be
mutated and invalid keys/values are rejected by either adding CEL validation
rules to the generated CRD for the Tags field (immutability check comparing to
oldObject.spec.tags and regex checks for keys/values) or implement webhook-based
validation on the Lease type by adding ValidateCreate and ValidateUpdate methods
that call the existing ValidateLeaseTags helper and return errors when tags
violate immutability or label rules; refer to the Tags field, the Lease type,
and the existing ValidateLeaseTags function when making these changes.
Comment on lines
+188
to
+192
| // LeasePolicyConfig defines policy constraints for leases. | ||
| type LeasePolicyConfig struct { | ||
| // Maximum number of user-defined tags allowed per lease. | ||
| // +kubebuilder:default=10 | ||
| MaxTags int32 `json:"maxTags,omitempty"` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the generated operator CRD contains the default and minimum after regenerating manifests.
rg -n -C3 'maxTags|leasePolicy|validation:Minimum|minimum:' controller/deploy/operatorRepository: jumpstarter-dev/jumpstarter
Length of output: 18645
Add +kubebuilder:validation:Minimum=0 to prevent negative lease policy tag limits.
The MaxTags field currently lacks a minimum constraint, allowing negative values that would cause runtime lease validation to reject all requests. This violates the pattern used for other numeric constraints in the file (Replicas, Port numbers).
Proposed fix
type LeasePolicyConfig struct {
// Maximum number of user-defined tags allowed per lease.
// +kubebuilder:default=10
+ // +kubebuilder:validation:Minimum=0
MaxTags int32 `json:"maxTags,omitempty"`
}After applying this fix, run make manifests generate from controller/deploy/operator to update the generated CRD.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/deploy/operator/api/v1alpha1/jumpstarter_types.go` around lines
188 - 192, Add a kubebuilder minimum validation tag to
LeasePolicyConfig.MaxTags: update the struct field for MaxTags by adding the
marker `+kubebuilder:validation:Minimum=0` (alongside the existing
`+kubebuilder:default=10`) so negative values are disallowed; then run `make
manifests generate` in controller/deploy/operator to refresh the generated CRD
artifacts.
Comment on lines
+149
to
+156
| tags: | ||
| additionalProperties: | ||
| type: string | ||
| description: |- | ||
| User-defined tags for the lease. Immutable after creation. | ||
| Maximum 10 entries. Keys and values must conform to Kubernetes label rules. | ||
| maxProperties: 10 | ||
| type: object |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the CRD file
find . -name "jumpstarter.dev_leases.yaml" -o -name "*leases.yaml" | head -20Repository: jumpstarter-dev/jumpstarter
Length of output: 254
🏁 Script executed:
# Read the CRD file to understand the structure around the tags field
cat controller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yamlRepository: jumpstarter-dev/jumpstarter
Length of output: 12249
🏁 Script executed:
# Check if x-kubernetes-validations already exist in this CRD
grep -n "x-kubernetes-validations" controller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yamlRepository: jumpstarter-dev/jumpstarter
Length of output: 112
Enforce the documented tag immutability.
The schema documents tags as immutable, but without a CEL validation rule in x-kubernetes-validations, direct Kubernetes updates via kubectl patch or API calls can still modify spec.tags, bypassing the documented constraint.
Add the following validation rule to prevent tag modifications after creation:
🛡️ Proposed validation rule
x-kubernetes-validations:
+ - message: tags are immutable after creation
+ rule: has(self.tags) == has(oldSelf.tags) && (!has(self.tags)
+ || self.tags == oldSelf.tags)
- message: one of selector or exporterRef.name is required🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yaml`
around lines 149 - 156, The CRD documents spec.tags as immutable but lacks a CEL
enforcement; add an x-kubernetes-validations entry that prevents changes to
spec.tags after creation by adding a rule such as: require that either oldSelf
is null (creation) or self.spec.tags == oldSelf.spec.tags (no modification),
with an appropriate message; insert this validation under the schema for tags
(the object with maxProperties: 10 and description) using the
x-kubernetes-validations key and reference spec.tags in the CEL expression to
block kubectl patch/API updates that attempt to change tags.
Comment on lines
+1306
to
+1310
| maxTags: | ||
| default: 10 | ||
| description: Maximum number of user-defined tags allowed per lease. | ||
| format: int32 | ||
| type: integer |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the current state of the operator Jumpstarters CRD file
cat -n controller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml | sed -n '1300,1320p'Repository: jumpstarter-dev/jumpstarter
Length of output: 1077
🏁 Script executed:
# Check the Lease CRD file for the maxProperties constraint
fd -t f "leases.yaml" controller/deploy/operator/config/crd/bases/Repository: jumpstarter-dev/jumpstarter
Length of output: 142
🏁 Script executed:
# Search for maxTags usage in the operator code
rg "maxTags|MaxTags" --type yaml --type go -A 3 -B 3Repository: jumpstarter-dev/jumpstarter
Length of output: 5977
🏁 Script executed:
# Check if there's validation logic for maxTags
rg "MaxTags" --type go -A 5 -B 5Repository: jumpstarter-dev/jumpstarter
Length of output: 6403
🏁 Script executed:
# Check the Lease CRD for the maxProperties constraint on tags
cat controller/deploy/operator/config/crd/bases/jumpstarter.dev_leases.yaml | grep -A 30 "tags:"Repository: jumpstarter-dev/jumpstarter
Length of output: 1525
🏁 Script executed:
# Also look for ValidateLeaseTags function implementation
cat -n controller/api/v1alpha1/lease_helpers.go | grep -A 20 "ValidateLeaseTags"Repository: jumpstarter-dev/jumpstarter
Length of output: 1117
🏁 Script executed:
# Check if there are any validation constraints in the jumpstarter_types.go
cat -n controller/deploy/operator/api/v1alpha1/jumpstarter_types.go | grep -B 5 -A 10 "type LeasePolicyConfig"Repository: jumpstarter-dev/jumpstarter
Length of output: 858
Add minimum: 1 and maximum: 10 constraints to the maxTags field.
The maxTags schema currently accepts negative values and values exceeding the Lease CRD's spec.tags.maxProperties: 10 limit. An admin could configure maxTags to a negative value (causing all requests to be rejected) or above 10 (a policy the Lease API cannot enforce). Constrain maxTags to the range [1, 10] to match the Lease CRD.
Proposed schema fix
maxTags:
default: 10
description: Maximum number of user-defined tags allowed per lease.
format: int32
+ minimum: 1
+ maximum: 10
type: integer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@controller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml`
around lines 1306 - 1310, The maxTags integer schema lacks bounds and can be set
negative or >10; update the CRD schema for the maxTags property (the maxTags
field in the operator.jumpstarter.dev Jumpstarter CRD) to include "minimum: 1"
and "maximum: 10" so its allowed range is [1,10] and matches the Lease CRD's
spec.tags.maxProperties constraint; keep the existing type/format/default but
add those two constraints to the maxTags schema block.
Comment on lines
+33
to
+38
| - **As a** CI pipeline author, **I want to** tag leases with my CI job | ||
| ID and pipeline name, **so that** I can correlate leases with builds. | ||
| - **As a** lab operator, **I want to** query leases by environment or | ||
| purpose, **so that** I can audit and manage lease usage. | ||
| - **As a** developer, **I want to** annotate leases | ||
|
|
Contributor
There was a problem hiding this comment.
Complete the third user story.
Line 37 is truncated and missing the benefit clause.
Suggested wording
-- **As a** developer, **I want to** annotate leases
+- **As a** developer, **I want to** annotate leases with purpose-specific metadata,
+ **so that** I can find and clean up my leases later.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **As a** CI pipeline author, **I want to** tag leases with my CI job | |
| ID and pipeline name, **so that** I can correlate leases with builds. | |
| - **As a** lab operator, **I want to** query leases by environment or | |
| purpose, **so that** I can audit and manage lease usage. | |
| - **As a** developer, **I want to** annotate leases | |
| - **As a** CI pipeline author, **I want to** tag leases with my CI job | |
| ID and pipeline name, **so that** I can correlate leases with builds. | |
| - **As a** lab operator, **I want to** query leases by environment or | |
| purpose, **so that** I can audit and manage lease usage. | |
| - **As a** developer, **I want to** annotate leases with purpose-specific metadata, | |
| **so that** I can find and clean up my leases later. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/internal/jeps/JEP-0012-lease-tags.md` around lines 33 - 38, The third
user story ("As a developer, I want to annotate leases") is truncated and
missing the benefit clause; update that bullet (the line starting "As a
developer, I want to annotate leases") to include the outcome/benefit — e.g.,
complete it as "As a developer, I want to annotate leases with metadata (owner,
ticket, reason), so that I can track intent and troubleshoot resource usage."
Ensure the completed sentence follows the same "As a ..., I want to ..., so that
..." format used by the other bullets.
Comment on lines
+64
to
+65
| - Key: max 63 characters, valid K8s label key format, must not use | ||
| `jumpstarter.dev/` or `metadata.jumpstarter.dev/` prefixes |
Contributor
There was a problem hiding this comment.
Clarify that tag keys are unprefixed names.
The server auto-prefixes tag keys with metadata.jumpstarter.dev/, so this should not describe user input as arbitrary Kubernetes label keys. Spell out that /-qualified keys are not accepted unless the implementation supports double-prefix-safe storage/querying.
Suggested clarification
-- Key: max 63 characters, valid K8s label key format, must not use
- `jumpstarter.dev/` or `metadata.jumpstarter.dev/` prefixes
+- Key: max 63 characters, valid unprefixed K8s label name format, and
+ must not contain `/` or use reserved prefixes such as `jumpstarter.dev/`
+ or `metadata.jumpstarter.dev/`Also applies to: 71-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/internal/jeps/JEP-0012-lease-tags.md` around lines 64 - 65, Update the
wording in JEP-0012 so tag keys are described as unprefixed label names (single
name segment, max 63 characters, DNS-1123 label/name rules) that must not
contain a '/' or an explicit prefix; clarify that the server will automatically
prepend the metadata.jumpstarter.dev/ prefix and users must provide only the
unprefixed key (apply the same change to the similar paragraph covering lines
71–84).
Validate tags server-side using K8s label rules. Store tags in LeaseSpec and as prefixed ObjectMeta labels for native K8s queryability. Add tag_filter support to ListLeases. Regenerate CRD manifests and restore lease RBAC permissions. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
bennyz
force-pushed
the
lease-tags-wt
branch
2 times, most recently
from
4700b5b to
59d4043
Compare
Add LeasePolicy section to the Jumpstarter operator CR with a maxTags field (default 10). Thread the value through the config chain: CR -> ConfigMap -> LoadConfiguration -> ClientService. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
Add --tag flag to 'jmp create lease' and --tag-filter to 'jmp get leases'. Wire tags through gRPC client, Lease model, and MCP tool. Display tags in Rich table output. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
Member
Author
|
Based on the meeting, maybe we need some "invisible" metadata (but filterable) as well |
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
1 participant
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.
For example:
Filtering