Add pkgMetadata label select on CRD - #1160
Conversation
✅ Deploy Preview for kpt-porch ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
dosubot
Bot
added
the
size:L
There was a problem hiding this comment.
Pull request overview
Adds v1alpha2 support for querying spec.packageMetadata.labels via standard Kubernetes label selectors by mirroring Kptfile labels onto the PackageRevision’s metadata.labels, and updates metadata sync semantics to “replace” so omitted keys are deleted.
Changes:
- Mirror Kptfile labels to
metadata.labelsusingporch.kpt.dev/kptfile-label__prefix with/escaping. - Switch spec→Kptfile metadata application from merge to replace (omitted keys removed), with new tests for deletion semantics.
- Add/expand e2e coverage and update docs for label-selector based filtering.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/crd/metadata_test.go | Adds e2e tests for label mirroring, slash-escaping, and deletion propagation behavior. |
| docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/inspecting-packages.md | Documents querying PackageRevisions via mirrored Kptfile label selectors. |
| controllers/packagerevisions/pkg/controllers/packagerevision/status.go | Implements label mirroring to metadata.labels and adjusts SSA spec apply behavior. |
| controllers/packagerevisions/pkg/controllers/packagerevision/status_test.go | Adds a unit test covering readiness gate removal semantics (needs adjustment for new label patching). |
| controllers/packagerevisions/pkg/controllers/packagerevision/metadata.go | Changes metadata application to replace semantics via applyMetadataMap. |
| controllers/packagerevisions/pkg/controllers/packagerevision/metadata_test.go | Updates unit tests to reflect replace semantics and clearing behavior. |
| controllers/packagerevisions/pkg/controllers/packagerevision/metadata_deletion_test.go | Adds additional tests for deletion/round-trip convergence (header comment needs update). |
Suppressed comments (2)
controllers/packagerevisions/pkg/controllers/packagerevision/status.go:289
- When Kptfile labels are removed and the object currently only has mirrored labels, this function returns
updated == nil. BecauseObjectMeta.Labelsisomitempty, applying with a nil map omits the labels field entirely, so SSA won't prune the previously-applied mirror label keys (they can get “stuck” on the object). Ensure an explicit empty map is applied when mirror labels need to be cleared and no other labels remain.
if len(kptfileLabels) == 0 {
// No Kptfile labels; remove any existing mirror labels from current.
var updated map[string]string
changed := false
for k, v := range current {
controllers/packagerevisions/pkg/controllers/packagerevision/status.go:307
- Mirrored labels are copied verbatim (after only '/' escaping) and unbounded. If a Kptfile label key/value is invalid for Kubernetes labels (length/charset),
applyObjectLabelswill fail and log repeatedly, and selectors won't work. Also consider capping mirrored entries (the linked issue’s acceptance criteria mentions a cap) to avoid unbounded growth of object metadata.
// Build desired Kptfile mirror labels: sort for determinism, then apply.
desired := make(map[string]string)
keys := slices.Sorted(maps.Keys(kptfileLabels))
for _, k := range keys {
// Escape "/" as "__" to fit Kubernetes label key constraints.
mirrorKey := kptfileLabelPrefix + strings.ReplaceAll(k, "/", "__")
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
efiacor
changed the title
Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech>
Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech>
Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech>
Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech>
Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech>
Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech>
eb59a7c to
3ebd84b
Compare
Signed-off-by: Fiachra Corcoran <fiachra.corcoran@est.tech>
|
| "--webhook-cert-dir=${workspaceFolder}/.build/deploy/.webhook-certs", | ||
| "-v=2" | ||
| ], | ||
| "cwd": "${workspaceFolder}", | ||
| "envFile": "${workspaceFolder}/.env", | ||
| "env": { | ||
| "GIT_CACHE_DIR": "${workspaceFolder}/.cache-controller-v1alpha2", | ||
| "DB_HOST": "${env:DB_HOST}", | ||
| "DB_PORT": "5432", | ||
| "DB_NAME": "porch", | ||
| "DB_USER": "porch", | ||
| "DB_PASSWORD": "porch", | ||
| "DB_DRIVER": "pgx", |
There was a problem hiding this comment.
any reason were messing with the launch.json here? like removal of DB_HOST? or did that just get pulled in by mistake?
There was a problem hiding this comment.
When getting the launch to work for controllers, I decided to move the ${env} ones out and move to use a local .env instead. Means we can each have our own gitignored .env file locally. I think we should do this for all launch targets, as having to set those (DB_HOST, FUNCTION_RUNNER_IP, etc) each time is messy. It's ok having a shared launch.json but ideally it shouldn't be commited to the repo.
Add PackageMetadata label selector support for v1alpha2 CRD
Description
What changed:
spec.packageMetadata.labelsto object metadata labels withporch.kpt.dev/kptfile-label__prefix__)Why it's needed:
spec.packageMetadata.labels[key]=valuevia field selectorskubectl -l porch.kpt.dev/kptfile-label__key=valueHow it works:
updateKptfileFields()extracts labels from Kptfile'sspec.packageMetadata.labelsapp.example.com/name) are escaped as__for Kubernetes compatibilityRelated Issue(s)
Type of Change
Checklist
Testing Instructions
spec.packageMetadata.labels: {env: prod, tier: backend}kubectl get pr -l porch.kpt.dev/kptfile-label__env=prodapp.example.com/nameand query withporch.kpt.dev/kptfile-label__app.example.com__nameAdditional Notes
AI Disclosure