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

⚠️ Make ip-address-manager an IPAM provider for CAPI #692

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

⚠️ Make ip-address-manager an IPAM provider for CAPI #692

wants to merge 1 commit into from

Conversation

peppi-lotta
Copy link
Member

@peppi-lotta peppi-lotta commented Sep 18, 2024 •
edited
Loading

This commit makes it possible to:

  • Deploy IPAM with clusterctl
  • Reconsile CAPI's ipaddressclaims with this managers ippools

I have two PR's, one in metal-dev-env and one in CAPM3 to mach these changes

All three PRs' changes need to be tested together. Check test in the metal-dev-env PR.

What this PR does / why we need it: To make this IPAM into a provider for CAPI

@metal3-io-bot metal3-io-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 18, 2024
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 7 times, most recently from 4fcd34f to abe0f52 Compare September 19, 2024 09:01
@peppi-lotta
Copy link
Member Author

/test metal3-ubuntu-e2e-integration-test-main
/test metal3-centos-e2e-integration-test-main

@peppi-lotta peppi-lotta mentioned this pull request Sep 19, 2024
Open
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 2 times, most recently from 32d5986 to 2f06f8b Compare September 26, 2024 08:15
@peppi-lotta peppi-lotta mentioned this pull request Oct 2, 2024
Open
Rozzii
Rozzii previously requested changes Oct 22, 2024
View reviewed changes
Copy link
Member

@Rozzii Rozzii left a comment

Choose a reason for hiding this comment

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

@peppi-lotta this is quite a big change so I prefer to review after all the CI checks are green.

Please fix the markdown and run the tests, then ping me please if everything is green.

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 5 times, most recently from 5fa145b to b610263 Compare October 24, 2024 07:02
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 2 times, most recently from 6b82fe0 to 8e486a5 Compare October 31, 2024 08:00
lentzi90
lentzi90 reviewed Nov 1, 2024
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

Great job!
We could probably use generics to avoid the double functions for everything. However, I think it is good to keep it like this for now since it means we know that the original code did not change.
Just minor things below about naming.

config/default/kustomization.yaml Outdated Show resolved Hide resolved
config/default/kustomization.yaml Outdated Show resolved Hide resolved
metadata.yaml Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
@Rozzii Rozzii modified the milestone: IPAM - v1.9 Nov 6, 2024
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 4 times, most recently from 027c714 to 1cb7909 Compare November 7, 2024 11:23
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch from 1cb7909 to 3407185 Compare November 7, 2024 11:26
@metal3-io-bot
Copy link
Contributor

metal3-io-bot commented Nov 7, 2024 •
edited
Loading

@peppi-lotta: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
metal3-ubuntu-e2e-integration-test-main abe0f52 link true /test metal3-ubuntu-e2e-integration-test-main
metal3-centos-e2e-integration-test-main abe0f52 link true /test metal3-centos-e2e-integration-test-main

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch from 3407185 to ab2256b Compare November 7, 2024 11:36
@Rozzii Rozzii self-requested a review November 8, 2024 08:19
@Rozzii Rozzii dismissed their stale review November 8, 2024 08:19

CI is green now

@Rozzii
Copy link
Member

Rozzii commented Nov 8, 2024

/hold
Feel free to review it and approve it but this feature will be merged after 1.9 release and will target 1.10 release.

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2024
@Rozzii Rozzii added this to the IPAM - v1.10 milestone Nov 8, 2024
lentzi90
lentzi90 reviewed Nov 13, 2024
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

I'm happy with this now!
/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lentzi90

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2024
tuminoid
tuminoid reviewed Nov 13, 2024
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

LGTM besides tiniest nits.

metadata.yaml Outdated Show resolved Hide resolved
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 3 times, most recently from 82c3dfa to a4fbb0b Compare November 15, 2024 11:10
@peppi-lotta
This commit makes it possible to:
fe5bc60 
- Deploy IPAM with clusterctl
- Reconsile CAPI's ipaddressclaims with this managers ippools

Signed-off-by: peppi-lotta <[email protected]>
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch from a4fbb0b to fe5bc60 Compare November 15, 2024 11:10
@tuminoid
Copy link
Member

I think this is pretty much done, so let's merge it when 1.9 is cut to get maximum amount of testing in 1.10 cycle.

Rozzii
Rozzii reviewed Nov 16, 2024
View reviewed changes
Copy link
Member

@Rozzii Rozzii left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rozzii Rozzii Rozzii left review comments

@lentzi90 lentzi90 lentzi90 left review comments

@tuminoid tuminoid tuminoid left review comments

@smoshiur1237 smoshiur1237 Awaiting requested review from smoshiur1237

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
Metal3 - Roadmap
Status: IPAM WIP
Milestone
IPAM - v1.10
Development

Successfully merging this pull request may close these issues.
5 participants
@peppi-lotta @metal3-io-bot @Rozzii @tuminoid @lentzi90