✨ Implement FromPoolAnnotation

[s3rj1k]

Implements: metal3-io/metal3-docs#571

[metal3-io-bot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[kashifest]

Thanks a lot @s3rj1k. Overall lgtm.

One minor comment. There's some core logic changes in the PR, such as the addFromAnnotation and getReferencedPools functions (which handle the extraction and processing of pool references from annotations at a lower level), do not appear to have direct unit test coverage in this PR. Would you please add a few more unit tests to cover those changes?

[s3rj1k]

Thanks a lot @s3rj1k. Overall lgtm.

One minor comment. There's some core logic changes in the PR, such as the addFromAnnotation and getReferencedPools functions (which handle the extraction and processing of pool references from annotations at a lower level), do not appear to have direct unit test coverage in this PR. Would you please add a few more unit tests to cover those changes?

Thanks for review, hopefully this will handle requested changes

[Rozzii]

I feel we need everyone to check this, quite a big change
@adilGhaffarDev @zaneb @fmuyassarov @Rozzii

[kashifest]

Thanks for review, hopefully this will handle requested changes

Thank you.
/approve

/hold
Since  @Rozzii asked more eyes on this PR, holding it for some time.

[metal3-io-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kashifest

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:

• OWNERS [kashifest]

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

[kashifest]

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

[s3rj1k]

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

Success!

[adilGhaffarDev]

Why are we allowing this to be set on any of the object?
why not just one, maybe just m3m?

[s3rj1k]

Few points:

• github.com/Add fromPoolAnnotation field for IPPool reference via annotation metal3-docs#571 this was explicitly showing a usecase where you would use BMH
• CAPM3 has other fromAnnotation features and they support all 3 objects
• as a consumer I want to have flexibility on what object I put this annotations, I don't want to be gated out on features
• as a consumer I create BMH, not m3m, I want all configs be in object that I create and don't want to be forced to ad-hoc update some ohter object that gets created during life-cycle of the cluster

What is the real concern here?

[adilGhaffarDev]

You’re right it works the same way for other fromAnnotation features. But since you’re not introducing that change, lgtm from my side on this PR.
In my opinion probably just bmh is fine, my concern is with the getValueFromAnnotation function, it currently prioritizes m3m and ignores other annotations. I dont think this is documented properly. I will check that.
/lgtm
cc @lentzi90 please check.

[s3rj1k]

https://github.com/metal3-io/cluster-api-provider-metal3/blob/main/baremetal/metal3data_manager.go#L1460

I am guessing that idea was something like, if we have some specific annotation managed in CAPM3 in future we would put that annotation on m3machine and let's ensure that this is what will be used, this kind of makes sense and I agree this should be documented if not already.

[Sunnatillo]

LGTM
Great work @s3rj1k. Thank you.

Is it possible for you to add documentation about how to use this feature in metal3 book?

[s3rj1k]

Is it possible for you to add documentation about how to use this feature in metal3 book?

We already have this metal3-io/metal3-docs@e6c0eac

Just needs to be updated, remove development part.
Maybe feature preview or something similar?

[s3rj1k]

@kashifest @Rozzii are we ready to land this?