Skip to content

Improve array field characteristics in API#7886

Open
erikgb wants to merge 1 commit intocert-manager:masterfrom
erikgb:set-api-list-type
Open

Improve array field characteristics in API#7886
erikgb wants to merge 1 commit intocert-manager:masterfrom
erikgb:set-api-list-type

Conversation

@erikgb
Copy link
Copy Markdown
Member

@erikgb erikgb commented Jul 29, 2025 •
edited
Loading

Pull Request Motivation

This is a follow-up after #7872, which included a report of API violations detected by the OpenAPI generator. Missing markers explicitly set to atomic were done in #7900. This PR suggests some changes that are technically breaking, but that could be performed as a bugfix.

This change will allow server-side apply to do the right thing when users are using SSA to manage cert-manager resources.

Kind

/kind bug

Release Note

Improve array field characteristics in APIs to improve support for server-side apply (SSA)

@cert-manager-prow cert-manager-prow bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. area/api Indicates a PR directly modifies the 'pkg/apis' directory area/deploy Indicates a PR modifies deployment configuration size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 29, 2025
@erikgb erikgb mentioned this pull request Jul 29, 2025
Closed
inteon
inteon reviewed Jul 31, 2025
View reviewed changes
inteon
inteon reviewed Jul 31, 2025
View reviewed changes
inteon
inteon reviewed Jul 31, 2025
View reviewed changes
inteon
inteon reviewed Jul 31, 2025
View reviewed changes
pkg/apis/certmanager/v1/types_certificate.go
// Requested DNS subject alternative names.
// +optional
// +listType=set
DNSNames []string `json:"dnsNames,omitempty"`
Copy link
Copy Markdown
Member

@inteon inteon Jul 31, 2025

Choose a reason for hiding this comment

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

Some users might depend on the ordering of DNS names in their x509 certificates.

Copy link
Copy Markdown
Member Author

@erikgb erikgb Aug 7, 2025

Choose a reason for hiding this comment

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

Hmmm, does order of DNS names in a certificate matter? Maybe @SgtCoDFish knows?

Copy link
Copy Markdown
Member Author

@erikgb erikgb Feb 13, 2026

Choose a reason for hiding this comment

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

@inteon, I don't think this affects the order at all. This is what ChatGPT responded:

Short answer: No — +listType=set does not affect sorting or ordering.
It affects equality semantics and merge behavior, not order.

And this is exactly what I am trying to improve in this PR.

inteon
inteon reviewed Jul 31, 2025
View reviewed changes
inteon
inteon reviewed Jul 31, 2025
View reviewed changes
inteon
inteon reviewed Jul 31, 2025
View reviewed changes
inteon
inteon reviewed Jul 31, 2025
View reviewed changes
@erikgb
Copy link
Copy Markdown
Member Author

erikgb commented Jul 31, 2025

I think we should wait with this PR until the upstream PR (kubernetes-sigs/gateway-api#3964) has landed.

/hold

@cert-manager-prow cert-manager-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 31, 2025
@erikgb erikgb force-pushed the set-api-list-type branch from b46af33 to 94b8cdb Compare August 2, 2025 20:05
@cert-manager-prow cert-manager-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2025
@erikgb erikgb changed the title Set missing array characteristics in API types WIP: Set missing array characteristics in API types Aug 6, 2025
@cert-manager-prow cert-manager-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 6, 2025
@erikgb
Copy link
Copy Markdown
Member Author

erikgb commented Aug 6, 2025

Learning from my upstream PR to do the same thing in Gateway API, I will create a new PR for just adding the missing markers. After that PR is merged, I will rebase this PR to allow us to continue the discussions on this PR.

@erikgb erikgb mentioned this pull request Aug 6, 2025
Merged
@erikgb erikgb changed the title WIP: Set missing array characteristics in API types WIP: Improve array field characteristics in API Aug 7, 2025
@erikgb erikgb force-pushed the set-api-list-type branch from 94b8cdb to 42f8e26 Compare August 7, 2025 11:01
@erikgb erikgb changed the title WIP: Improve array field characteristics in API Improve array field characteristics in API Aug 7, 2025
@cert-manager-prow cert-manager-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 7, 2025
@erikgb erikgb force-pushed the set-api-list-type branch from 42f8e26 to d4e27ae Compare August 7, 2025 11:08
@erikgb erikgb requested a review from SgtCoDFish August 7, 2025 11:14
@erikgb erikgb force-pushed the set-api-list-type branch 3 times, most recently from 6a5741b to 4fb2ec2 Compare August 7, 2025 18:02
@cert-manager-prow cert-manager-prow bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 7, 2025
@erikgb
Copy link
Copy Markdown
Member Author

erikgb commented Aug 7, 2025

/kind bug

@cert-manager-prow cert-manager-prow bot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 7, 2025
@erikgb
Improve array field characteristics in API
cb3702e 
Signed-off-by: Erik Godding Boye <egboye@gmail.com>
@cert-manager-prow
Copy link
Copy Markdown
Contributor

cert-manager-prow bot commented Feb 13, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign munnerz for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@erikgb
Copy link
Copy Markdown
Member Author

erikgb commented Feb 13, 2026

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@inteon inteon inteon left review comments

@SgtCoDFish SgtCoDFish Awaiting requested review from SgtCoDFish

Assignees

No one assigned

Labels

area/api Indicates a PR directly modifies the 'pkg/apis' directory area/deploy Indicates a PR modifies deployment configuration dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@erikgb @inteon