Skip to content

Fix long CRD change group validation #1124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
guettli wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fix long CRD change group validation #1124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions pkg/kapp/diffgraph/change_group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,9 @@ func (r ChangeGroup) isQualifiedNameWithoutLen(name string) []string {
errStrs := k8sval.IsQualifiedName(name)
var updatedErrStrs []string
for _, err := range errStrs {
// Allow change group names to have more characters than the default maxLength
if !strings.Contains(err, k8sval.MaxLenError(k8sval.DNS1035LabelMaxLength)) {
// 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)
}
Comment on lines +55 to 59
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}
Expand Down
2 changes: 2 additions & 0 deletions pkg/kapp/diffgraph/change_group_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ func TestNewChangeGroupFromAnnString(t *testing.T) {
"valid-name.com/valid-name_CustomResourceDefinition--valid",
// Example from pinniped of a long name
"change-groups.kapp.k14s.io/crds-authentication.concierge.pinniped.dev-WebhookAuthenticator",
// Regression test for #1123: long CRD group + kind can exceed 63 bytes.
"change-groups.kapp.k14s.io/crds-fooooooooooooooooooooooooooooooooooooooooooooooooooooo.example.com-SomeLongCRDName",
Comment on lines +25 to +26
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}
for _, name := range names {
cg, err := ctldgraph.NewChangeGroupFromAnnString(name)
Expand Down
Loading