Skip to content

crd, Status.IPs: Add omitempty, MinItems attributes#11

Closed
RamLavi wants to merge 1 commit into
k8snetworkplumbingwg:mainfrom
RamLavi:status_ips_omitempty
Closed

crd, Status.IPs: Add omitempty, MinItems attributes#11
RamLavi wants to merge 1 commit into
k8snetworkplumbingwg:mainfrom
RamLavi:status_ips_omitempty

Conversation

@RamLavi

@RamLavi RamLavi commented Oct 23, 2025

Copy link
Copy Markdown
Member

This PR is setting the omitempty and MinItems=1 to the IPs field.

Reason: it is essintial to be able to set status attributes (such as conditions) even if the IPs field is empty.
This requires making the attribute omitempty.
Setting the MinItems=1 is meant to avoid setting an empty slice which is confusing according to the API team.

@RamLavi RamLavi changed the title crd: add omitempty attribute to Status.IPs name crd: add omitempty attribute to Status.IPs Oct 23, 2025
@RamLavi RamLavi changed the title crd: add omitempty attribute to Status.IPs crd: Add omitempty attribute to Status.IPs Oct 23, 2025

@maiqueb maiqueb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this essential ? Can't you just set an empty slice ?

It is essintial to be able to set status attributes (such as conditions)
even if the IPs field is empty. This requires making the attribute
`omitempty`.

Signed-off-by: Ram Lavi <ralavi@redhat.com>
@RamLavi RamLavi force-pushed the status_ips_omitempty branch from b7bef6a to 1939fe4 Compare October 23, 2025 11:22

@maiqueb maiqueb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is better.

But I need to see some test checking this out.

@ormergi

ormergi commented Oct 23, 2025

Copy link
Copy Markdown

@maiqueb

maiqueb commented Oct 23, 2025

Copy link
Copy Markdown
Collaborator

I think we need a complementary MaxItems as well. And you can also validating the IP itself, using isIP/isCIDR, for example https://github.com/ovn-kubernetes/ovn-kubernetes/blob/master/go-controller/pkg/crd/userdefinednetwork/v1/shared.go#L241-L242 https://github.com/ovn-kubernetes/ovn-kubernetes/blob/master/go-controller/pkg/crd/userdefinednetwork/v1/shared.go#L232-L234

That's off scope.

Not saying it doesn't make sense, just saying it has nothing to do with what we're attempting to do here.

And while saying that, I'm against setting MaxItems in here. The fact you can have at most 2 IPs, one from each family is a limitation I don't want to impose on this CRD.

@RamLavi

RamLavi commented Oct 27, 2025

Copy link
Copy Markdown
Member Author

This is better.
But I need to see some test checking this out.

@maiqueb adding this PR on top of ovn-kubernetes/ovn-kubernetes#5683 (see commit) makes the e2e pass (i.e. before- failing, after - passing).
I hope this qualifies as checking it out.

@RamLavi RamLavi changed the title crd: Add omitempty attribute to Status.IPs crd: Add omitempty, MinItems attributes to Status.IPs Oct 27, 2025
@RamLavi RamLavi changed the title crd: Add omitempty, MinItems attributes to Status.IPs crd, Status.IPs: Add omitempty, MinItems attributes Oct 27, 2025
@openshift-merge-robot

Copy link
Copy Markdown

Fix included in accepted release 4.22.0-0.nightly-2026-01-06-164201

@RamLavi

RamLavi commented Jan 8, 2026

Copy link
Copy Markdown
Member Author

closing as this change was not accepted by the community

@RamLavi RamLavi closed this Jan 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants