Skip to content

Add k8s:customUnique to unblock enabling listmap uniqueness#188

Merged
yongruilin merged 7 commits intojpbetz:mainfrom
yongruilin:vg_csr-listmap-unique
Sep 22, 2025
Merged

Add k8s:customUnique to unblock enabling listmap uniqueness#188
yongruilin merged 7 commits intojpbetz:mainfrom
yongruilin:vg_csr-listmap-unique

Conversation

@yongruilin
Copy link
Copy Markdown
Collaborator

@yongruilin yongruilin commented Sep 16, 2025

No description provided.

@yongruilin yongruilin mentioned this pull request Sep 16, 2025
Open
thockin
thockin reviewed Sep 16, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@thockin thockin left a comment

Choose a reason for hiding this comment

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

As a POC this looks really good. I didn't think I would like it using +unique and I was right :)

Ideas for a better name, or good arguments why it SHOULD use +unique?

Comment thread staging/src/k8s.io/code-generator/cmd/validation-gen/validators/list.go Outdated
@yongruilin yongruilin force-pushed the vg_csr-listmap-unique branch from 3569593 to b2295fb Compare September 16, 2025 22:14
@yongruilin
Copy link
Copy Markdown
Collaborator Author

yongruilin commented Sep 16, 2025

Ideas for a better name, or good arguments why it SHOULD use +unique?

  • k8s:customUnique or `k8s:customUniqueness : I think it sounds like a plan.
  • k8s:omitDefaultUniqueness : A bit too long?
  • k8s:handwrittenUnique
  • k8s:NoUnique : i don't like this one, as it is not accurate as handwritten may have uniqueness check

Overall, I prefer k8s:customUnique. I'm now thinking we should NOT overload the k8s:unique tag. The k8s:customUnique tag (as a standalone boolean-style tag) is safer and cleaner because it's a completely new, unambiguous marker

@yongruilin yongruilin force-pushed the vg_csr-listmap-unique branch 3 times, most recently from 473c37f to fe47bfa Compare September 17, 2025 22:52
@yongruilin yongruilin changed the title [POC] Add unique=override to enable listmap uniqueness [POC] Add k8s:customUnique to enable listmap uniqueness Sep 18, 2025
@yongruilin yongruilin changed the title [POC] Add k8s:customUnique to enable listmap uniqueness [POC] Add k8s:customUnique to unblock enabling listmap uniqueness Sep 18, 2025
thockin
thockin reviewed Sep 20, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@thockin thockin left a comment

Choose a reason for hiding this comment

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

I am LGTM on this - let's give time for Joe or Aaron to look at it, but if no comments , you can self-merge :)

@thockin thockin changed the title [POC] Add k8s:customUnique to unblock enabling listmap uniqueness Add k8s:customUnique to unblock enabling listmap uniqueness Sep 20, 2025
@jpbetz
Copy link
Copy Markdown
Owner

jpbetz commented Sep 20, 2025

LGTM, I think the name conveys enough information here. If I saw a tag like this for the first time, I'd want to make sure I read the docs to understand what exactly it means.

@yongruilin yongruilin force-pushed the vg_csr-listmap-unique branch from fe47bfa to 88fc288 Compare September 22, 2025 16:52
@aaron-prindle
Copy link
Copy Markdown
Collaborator

aaron-prindle commented Sep 22, 2025

LGTM

@yongruilin yongruilin merged commit 61c0b58 into jpbetz:main Sep 22, 2025
1 check passed
@yongruilin yongruilin mentioned this pull request Sep 25, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aaron-prindle aaron-prindle aaron-prindle left review comments

@thockin thockin thockin left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yongruilin @jpbetz @aaron-prindle @thockin