Skip to content
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.

Sign up for GitHub

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

Jump to bottom

apis: increase max lengths of ips to 2 #80

Merged
merged 2 commits into from
Jan 16, 2025
Merged

apis: increase max lengths of ips to 2 #80

merged 2 commits into from
Jan 16, 2025

Conversation

MrFreezeex
Copy link
Member

@MrFreezeex MrFreezeex commented Oct 4, 2024 •
edited
Loading

Update the Service controller to update the ServiceImport with all the IPs instead of only the first IP.

A bit related to #63 might not be the complete answer for this though

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 4, 2024
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 4, 2024
mikemorris
mikemorris approved these changes Dec 10, 2024
View reviewed changes
Copy link

@mikemorris mikemorris left a comment •
edited
Loading

Choose a reason for hiding this comment

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

/approve
/hold

Pending comments in https://github.com/kubernetes-sigs/mcs-api/pull/80/files#r1878240881, should ensure #85 merges first.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 10, 2024
@MrFreezeex MrFreezeex force-pushed the svcoperator-multipleips branch from d8fd703 to d6977cb Compare January 13, 2025 14:47
@MrFreezeex
apis: increase max lengths of ips to 2
10096be 
Bump the max number of IPs to 2 instead of 1. This is to reflect the
maximum number of IPs in regular Services and to be dual stack
compatible.

Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>
@MrFreezeex MrFreezeex force-pushed the svcoperator-multipleips branch from d6977cb to a711865 Compare January 13, 2025 14:48
@MrFreezeex
Copy link
Member Author

MrFreezeex commented Jan 13, 2025 •
edited
Loading

It's been a while that we are discussing v1alpha2 but not going forward with it. And this is a non breaking change so I would like to do this change + the related api change to v1alpha1 here instead of waiting for v1alpha2 if that's ok for you.

cc @tpantelis @mikemorris @skitt 🙏

@MrFreezeex MrFreezeex changed the title Update service controller to add all the IPs from derived service pis: increase max lengths of ips to 2 Jan 13, 2025
@MrFreezeex
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 13, 2025
@MrFreezeex MrFreezeex changed the title pis: increase max lengths of ips to 2 apis: increase max lengths of ips to 2 Jan 13, 2025
@mikemorris
Copy link

Sorry I've been behind following up on your work! This feels okay to me, may need to regenerate the CRD YAML in this PR now though.

@MrFreezeex
Copy link
Member Author

/test pull-mcs-api-e2e

@MrFreezeex MrFreezeex force-pushed the svcoperator-multipleips branch from a711865 to f9bf777 Compare January 13, 2025 15:14
@MrFreezeex
Copy link
Member Author

Sorry I've been behind following up on your work! This feels okay to me, may need to regenerate the CRD YAML in this PR now though.

No worries, actually I have already regenerated the yaml so it should be fine on that aspect 👍

@tpantelis
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2025
@MrFreezeex
Copy link
Member Author

Hmm I can't reproduce the CI failure locally let's try a bit more 😅
/test pull-mcs-api-e2e

@MrFreezeex
Copy link
Member Author

/retest-required

@MrFreezeex
Copy link
Member Author

/test pull-mcs-api-e2e

@skitt
Copy link
Member

skitt commented Jan 16, 2025

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikemorris, MrFreezeex, skitt

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

The pull request process is described here

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2025
@k8s-ci-robot k8s-ci-robot merged commit e786573 into kubernetes-sigs:master Jan 16, 2025
6 checks passed
@MrFreezeex MrFreezeex mentioned this pull request Jan 16, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tpantelis tpantelis tpantelis left review comments

@mikemorris mikemorris mikemorris approved these changes

@lauralorenz lauralorenz Awaiting requested review from lauralorenz

@skitt skitt Awaiting requested review from skitt

Assignees

@tpantelis tpantelis

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MrFreezeex @mikemorris @tpantelis @skitt @k8s-ci-robot