-
Notifications
You must be signed in to change notification settings - Fork 180
K8SPSMDB-1593: Refactor user annotation key generation #2232
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.
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
3b8164d
d0c1a21
b0deb59
3c13d52
53ae4bb
5dc6fa5
cba96c1
78a982d
e81adde
bb80a94
f148275
d67a311
5e0a03a
e4288fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ package perconaservermongodb | |
|
|
||
| import ( | ||
| "context" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/pkg/errors" | ||
|
|
@@ -411,3 +412,98 @@ func TestGetCustomUserSecret(t *testing.T) { | |
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestBuildAnnotationKey(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| crName string | ||
| userName string | ||
| want string | ||
| wantLen int | ||
| maxLength int | ||
| }{ | ||
| { | ||
| name: "short names within limit", | ||
| crName: "my-cluster", | ||
| userName: "user1", | ||
| want: "percona.com/my-cluster-user1-hash", | ||
| wantLen: 33, | ||
| maxLength: 63, | ||
| }, | ||
| { | ||
| name: "exactly at limit", | ||
| crName: "a", | ||
| userName: strings.Repeat("x", 44), // 1 + 5 + 44 + 13 (percona.com/-) = 63, will not be truncated | ||
| want: "percona.com/a-" + strings.Repeat("x", 44) + "-hash", | ||
| wantLen: 63, | ||
| maxLength: 63, | ||
| }, | ||
| { | ||
| name: "exceeds limit - truncates but keeps hash suffix", | ||
| crName: "very-long-cluster-name-that-exceeds", | ||
| userName: "very-long-user-name-that-also-exceeds", | ||
| want: "percona.com/very-long-cluster-name-that-exceeds-very-long--hash", | ||
| wantLen: 63, | ||
| maxLength: 63, | ||
| }, | ||
| { | ||
| name: "very long cluster name", | ||
| crName: strings.Repeat("a", 100), | ||
| userName: "user", | ||
| want: "percona.com/" + strings.Repeat("a", 46) + "-hash", | ||
| wantLen: 63, | ||
| maxLength: 63, | ||
| }, | ||
| { | ||
| name: "very long user name", | ||
| crName: "cluster", | ||
| userName: strings.Repeat("b", 100), | ||
| want: "percona.com/cluster-" + strings.Repeat("b", 38) + "-hash", | ||
| wantLen: 63, | ||
| maxLength: 63, | ||
| }, | ||
| { | ||
| name: "both names very long", | ||
| crName: strings.Repeat("c", 50), | ||
| userName: strings.Repeat("d", 50), | ||
| want: "percona.com/" + strings.Repeat("c", 46) + "-hash", | ||
| wantLen: 63, | ||
| maxLength: 63, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| got := buildAnnotationKey(tt.crName, tt.userName) | ||
|
|
||
| // Extract the name part (after "percona.com/") | ||
| prefix := "percona.com/" | ||
| if !strings.HasPrefix(got, prefix) { | ||
| t.Errorf("buildAnnotationKey() = %v, should start with %v", got, prefix) | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of using these conditions, we can use assert and require from testify as we do in other tests in this repo.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the same of some other cases in the same test
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ravisingal could you please check these comments
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated tests. |
||
|
|
||
| namePart := got[len(prefix):] | ||
| gotLen := len(got) | ||
|
|
||
| // Verify the annotation key name part is within Kubernetes limit | ||
| if len(namePart) > tt.maxLength { | ||
| t.Errorf("buildAnnotationKey() name part length = %v, should be <= %v. Got: %v", len(namePart), tt.maxLength, got) | ||
| } | ||
|
|
||
| // Verify it ends with "-hash" | ||
| if !strings.HasSuffix(got, "-hash") { | ||
| t.Errorf("buildAnnotationKey() = %v, should end with '-hash'", got) | ||
| } | ||
|
|
||
| // Verify exact match for non-truncated cases | ||
| if gotLen <= tt.maxLength && tt.want != "" { | ||
| assert.Equal(t, tt.want, got, "buildAnnotationKey() = %v, want %v", got, tt.want) | ||
| } | ||
|
|
||
| // Verify length matches expected for all cases | ||
| if tt.wantLen > 0 { | ||
| assert.Equal(t, tt.wantLen, gotLen, "buildAnnotationKey() length = %v, want %v", gotLen, tt.wantLen) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we need to move this to outside of function and use it in unit tests instead of passing the same value separately in both places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.