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

✨ Deprecate NoCloudProvider and add CloudProviderEnabled #2108

Merged

metal3-io-bot merged 1 commit into metal3-io:main from Nordix:deprecate-noCloudProvider/adil

Jan 23, 2025

Member

adilGhaffarDev commented Nov 22, 2024 •

edited

Loading

What this PR does / why we need it:
Deprecate NoCloudProvider add CloudProviderEnabled, and set the default value of CloudProviderEnabled to true.

NoCloudProvider was bool and defaulted to false whenever it was accessed. This PR is changing both NoCloudProvider and adding CloudProviderEnabled to *bool so we can easily see if the user changes the value. We need to check if the value is changed by the user or not to check the conflict between these variables.
We are setting the default value of CloudProviderEnabled to true because this was the default behavior of NoCloudProvider in CAPM3 when it was added.

There is a plan to fix this defaulting when we remove NoCloudProvider completely. Check the linked issue.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #2255

metal3-io-bot requested review from kashifest and tuminoid

November 22, 2024 07:50

metal3-io-bot added the size/M label

adilGhaffarDev force-pushed the deprecate-noCloudProvider/adil branch from 7f7a168 to 18178b7 Compare

November 22, 2024 08:11

Member Author

adilGhaffarDev commented Nov 22, 2024

Member

tuminoid commented Nov 22, 2024

cc @lentzi90

Btw use
/cc @lentzi90
So instead of just pinging him, it actually requests review.

metal3-io-bot requested a review from lentzi90

November 22, 2024 08:53

lentzi90 reviewed

View reviewed changes

Member

lentzi90 left a comment

Thanks for working on this!
I think we need to add some validation or other logic to make sure we don't end up in confusing situations where these two contradict each other. Do you think it is possible to do this and also add a testcase for it?
I'm not sure if it is best to do this in the validating webhook or if we can somehow detect later which one of them were set explicitly.

api/v1beta1/metal3cluster_types.go Outdated Show resolved Hide resolved

adilGhaffarDev force-pushed the deprecate-noCloudProvider/adil branch from 18178b7 to 476a183 Compare

January 21, 2025 08:33

metal3-io-bot added needs-rebase size/L and removed size/M labels

adilGhaffarDev force-pushed the deprecate-noCloudProvider/adil branch from 476a183 to 8fc1141 Compare

January 21, 2025 08:37

metal3-io-bot removed the needs-rebase label

adilGhaffarDev force-pushed the deprecate-noCloudProvider/adil branch from 8fc1141 to 5cd0acc Compare

January 21, 2025 11:02

lentzi90 reviewed

View reviewed changes

Member

lentzi90 left a comment

/approve

Contributor

metal3-io-bot commented Jan 21, 2025

[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:

~~OWNERS~~ [lentzi90]

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 added the approved label

Member

tuminoid commented Jan 21, 2025

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

tuminoid reviewed

View reviewed changes

Member

tuminoid left a comment

Nits.

Do we really need to put that comment everywhere?

api/v1beta1/metal3cluster_types.go Outdated Show resolved Hide resolved

api/v1beta1/metal3cluster_types.go Show resolved Hide resolved

config/crd/bases/infrastructure.cluster.x-k8s.io_metal3clusters.yaml Outdated Show resolved Hide resolved

config/crd/bases/infrastructure.cluster.x-k8s.io_metal3clustertemplates.yaml Outdated Show resolved Hide resolved

docs/api.md Show resolved Hide resolved

examples/cluster/cluster.yaml Outdated Show resolved Hide resolved

examples/clusterctl-templates/clusterctl-cluster.yaml Outdated Show resolved Hide resolved

examples/templates/clusterclass.yaml Outdated Show resolved Hide resolved

test/e2e/data/infrastructure-metal3/bases/cluster/cluster-with-kcp.yaml Outdated Show resolved Hide resolved

test/e2e/data/infrastructure-metal3/bases/clusterclass-cluster/cluster-with-kcp.yaml Outdated Show resolved Hide resolved

adilGhaffarDev force-pushed the deprecate-noCloudProvider/adil branch from 5cd0acc to c78073b Compare

January 21, 2025 13:20

Member Author

adilGhaffarDev commented Jan 21, 2025

Do we really need to put that comment everywhere?

Perhaps not, I have removed it from test yamls and some other places where it was not necessary.

Member

tuminoid commented Jan 21, 2025

Thanks, LGTM.

Let's run tests for it.
/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

adilGhaffarDev force-pushed the deprecate-noCloudProvider/adil branch from c78073b to a16844f Compare

January 21, 2025 15:38

Member Author

adilGhaffarDev commented Jan 21, 2025

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

1 similar comment

Member Author

adilGhaffarDev commented Jan 21, 2025

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

Member

tuminoid commented Jan 21, 2025

Should we make this PR title sparkles? ✨

adilGhaffarDev changed the title ~~🌱 Deprecate NoCloudProvider and add CloudProviderEnabled~~ ✨ Deprecate NoCloudProvider and add CloudProviderEnabled

mquhuy reviewed

View reviewed changes

Member

mquhuy left a comment

Nits.

api/v1beta1/metal3cluster_webhook.go Outdated

@@ @@ -68,6 +74,79 @@ func (c *Metal3Cluster) validate() error { @@
               		)
               	}
+              	if c.Spec.CloudProviderEnabled != nil && c.Spec.NoCloudProvider != nil {
+              		if !*c.Spec.CloudProviderEnabled && !*oldM3C.Spec.NoCloudProvider {

Member

mquhuy Jan 22, 2025 •

edited

Loading

We can merge the two ifs in this block to if *c.Spec.CloudProviderEnabled == *oldM3C.Spec.NoCloudProvider, and what if oldM3C == nil?

Member Author

adilGhaffarDev Jan 22, 2025

oh, this is a mistake it should be
if !*c.Spec.CloudProviderEnabled && !*c.Spec.NoCloudProvider {
Let me fix

Member Author

adilGhaffarDev Jan 22, 2025

fixed

api/v1beta1/metal3cluster_webhook.go Show resolved Hide resolved

adilGhaffarDev force-pushed the deprecate-noCloudProvider/adil branch from a16844f to 4a79c35 Compare

January 22, 2025 09:05

mquhuy reviewed

View reviewed changes

Member

mquhuy left a comment

Thanks. Still some nits.
Honestly I think it would be best if you could put this repetitive logic into a function, but that's up to you.

api/v1beta1/metal3cluster_webhook.go Outdated

@@ @@ -68,6 +74,79 @@ func (c *Metal3Cluster) validate() error { @@
               		)
               	}
+              	if c.Spec.CloudProviderEnabled != nil && c.Spec.NoCloudProvider != nil {
+              		if !*c.Spec.CloudProviderEnabled && !*c.Spec.NoCloudProvider {

Member

mquhuy Jan 22, 2025

if !*c.Spec.CloudProviderEnabled && !*c.Spec.NoCloudProvider and if *c.Spec.CloudProviderEnabled && *c.Spec.NoCloudProvider can be combined to if *c.Spec.CloudProviderEnabled == *c.Spec.NoCloudProvider.

Member Author

adilGhaffarDev Jan 22, 2025

done

api/v1beta1/metal3cluster_webhook.go Outdated

+              	if oldM3C != nil {
+              		// Validate cloudProviderEnabled
+              		if c.Spec.CloudProviderEnabled != nil && oldM3C.Spec.NoCloudProvider != nil {
+              			if !*c.Spec.CloudProviderEnabled && !*oldM3C.Spec.NoCloudProvider {

Member

mquhuy Jan 22, 2025

Same here. if *c.Spec.CloudProviderEnabled == *oldM3C.Spec.NoCloudProvider

api/v1beta1/metal3cluster_webhook.go Outdated

+              		// Validate noCloudProvider
+              		if c.Spec.NoCloudProvider != nil && oldM3C.Spec.CloudProviderEnabled != nil {
+              			if !*c.Spec.NoCloudProvider && !*oldM3C.Spec.CloudProviderEnabled {

Member

mquhuy Jan 22, 2025

Same here.


          Deprecate NoCloudProvider and add CloudProviderEnabled

5e733bd

Signed-off-by: Muhammad Adil Ghaffar <[email protected]>

adilGhaffarDev force-pushed the deprecate-noCloudProvider/adil branch from 4a79c35 to 5e733bd Compare

January 22, 2025 10:37

Member

mquhuy commented Jan 22, 2025

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

Member

tuminoid commented Jan 22, 2025

/retest
Network flake

Member

mquhuy commented Jan 23, 2025

LGTM

tuminoid reviewed

View reviewed changes

Member

tuminoid left a comment

/lgtm

metal3-io-bot added the lgtm label

metal3-io-bot merged commit b4480cf into metal3-io:main

18 checks passed

metal3-io-bot added this to the CAPM3 - v1.10 milestone

metal3-io-bot deleted the deprecate-noCloudProvider/adil branch

January 23, 2025 10:31

tuminoid mentioned this pull request

CloudProviderEnabled feature in CAPM3 broke dev-env tests metal3-io/metal3-dev-env#1499

Closed

adilGhaffarDev mentioned this pull request

🌱 remove kubebuilder:default=true for CloudProviderEnabled and set defaults in defaulting webhook #2276

Merged

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

Labels

approved lgtm size/L