Fix long CRD change group validation#1124
Conversation
Signed-off-by: Thomas Güttler <thomas.guettler@syself.com>
guettli
force-pushed
the
fix-1123-long-crd-change-group-name
branch
from
fc904f4 to
fd9f944
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression in ChangeGroup annotation validation by tolerating Kubernetes’ max-length error message variants (e.g., “characters” vs “bytes”), restoring support for long CRD-derived change-group names, and adds a regression test for the reported failure (Issue #1123).
Changes:
- Update change-group qualified-name validation to ignore 63-length max-len errors regardless of Kubernetes wording (“characters”/“bytes”).
- Add a regression test case for a long CRD-derived change-group name that exceeds the 63-byte name-part limit.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/kapp/diffgraph/change_group.go | Adjusts validation error filtering to ignore max-length failures across Kubernetes message variants. |
| pkg/kapp/diffgraph/change_group_test.go | Adds a regression test covering long CRD-derived change-group names. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Allow change group names to have more characters than the default maxLength. | ||
| // k8s has used both "characters" and "bytes" wording across versions. | ||
| if !strings.Contains(err, "must be no more than 63 ") { | ||
| updatedErrStrs = append(updatedErrStrs, err) | ||
| } |
There was a problem hiding this comment.
The length-error filter is currently based on a hard-coded substring ("must be no more than 63 "), which is brittle and may accidentally suppress unrelated validation errors that happen to mention that length. Consider restricting the match to the specific "name part" max-len error and deriving the number from the Kubernetes constant (e.g., DNS1035LabelMaxLength) instead of embedding 63 in the string.
| // Regression test for #1123: long CRD group + kind can exceed 63 bytes. | ||
| "change-groups.kapp.k14s.io/crds-fooooooooooooooooooooooooooooooooooooooooooooooooooooo.example.com-SomeLongCRDName", |
There was a problem hiding this comment.
This regression test relies on a manually-constructed long name. To avoid the test becoming a no-op if the literal is shortened/edited later, consider generating the long segment (e.g., via strings.Repeat) and/or asserting that the annotation's name part actually exceeds the 63-byte limit before calling NewChangeGroupFromAnnString.
Summary
Why
kapp allows long CRD-derived change-group names, but newer Kubernetes validation returns a length error string with "bytes" wording that was no longer filtered, causing regressions.
Closes #1123.