K8SPSMDB-1593: Refactor user annotation key generation#2232
Open
ravisingal wants to merge 10 commits intopercona:mainfrom
Open
K8SPSMDB-1593: Refactor user annotation key generation#2232ravisingal wants to merge 10 commits intopercona:mainfrom
ravisingal wants to merge 10 commits intopercona:mainfrom
Conversation
Refactor annotation key generation to ensure it adheres to Kubernetes naming constraints by limiting the prefix length.
ravisingal
requested review from
egegunes,
gkech,
hors,
mayankshah1607,
nmarukovich,
oksana-grishchenko and
pooknull
as code owners
|
|
egegunes
requested changes
Feb 11, 2026
Contributor
egegunes
left a comment
egegunes
left a comment
There was a problem hiding this comment.
@ravisingal it'd be great to have some unit tests for this
Author
added unit tests |
egegunes
requested changes
Feb 18, 2026
| func buildAnnotationKey(crName, userName string) string { | ||
| annotationKeyBase := fmt.Sprintf("percona.com/%s-%s", crName, userName) | ||
| const hashSuffix = "-hash" | ||
| const maxAnnotationNameLength = 63 |
Contributor
There was a problem hiding this comment.
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
egegunes
changed the title
egegunes
previously approved these changes
Feb 23, 2026
gkech
reviewed
Mar 6, 2026
Comment on lines
+481
to
+483
| if !strings.HasPrefix(got, prefix) { | ||
| t.Errorf("buildAnnotationKey() = %v, should start with %v", got, prefix) | ||
| } |
Contributor
There was a problem hiding this comment.
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.
the same of some other cases in the same test
Collaborator
There was a problem hiding this comment.
@ravisingal could you please check these comments
mayankshah1607
previously approved these changes
Mar 6, 2026
hors
reviewed
Mar 10, 2026
Collaborator
hors
left a comment
hors
left a comment
There was a problem hiding this comment.
@ravisingal please sign cla
ravisingal
dismissed stale reviews from egegunes and mayankshah1607
via
e81adde
Author
done. |
Collaborator
commit: bb80a94 |
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
7 participants
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.
Refactor annotation key generation to ensure it adheres to Kubernetes naming constraints by limiting the prefix length.
CHANGE DESCRIPTION
Problem:
Short explanation of the problem.
Cause:
Short explanation of the root cause of the issue if applicable.
Solution:
Short explanation of the solution we are providing with this PR.
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
compare/*-oc.yml)?Config/Logging/Testability