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.

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

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

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

adilGhaffarDev
Copy link
Member

@adilGhaffarDev adilGhaffarDev commented Jan 24, 2025

What this PR does / why we need it:
Instead of using // +kubebuilder:default=true to set a default value for CloudProviderEnabled, we are configuring defaults through a defaulting webhook. This approach avoids conflicts with the NoCloudProvider field in the validation webhook.

Using kubebuilder:default makes it impossible to distinguish whether the value of CloudProviderEnabled was explicitly set by the user or automatically assigned by the default. This distinction is critical because our validation webhook ensures that CloudProviderEnabled and NoCloudProvider do not conflict when both are set. If we rely on kubebuilder:default, the validation webhook cannot function as intended, as it cannot determine the source of the CloudProviderEnabled value. By shifting defaulting logic to the webhook, we retain the ability to enforce these validation rules effectively.

This PR is a follow-up of this PR: #2108

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

@metal3-io-bot metal3-io-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 24, 2025
@adilGhaffarDev adilGhaffarDev force-pushed the fix-noccloudprovider/adil branch from bdeeef6 to 005857d Compare January 27, 2025 11:06
@metal3-io-bot metal3-io-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 27, 2025
@adilGhaffarDev adilGhaffarDev changed the title 🌱 More precise logs to understand failure[testing] 🌱 remove kubebuilder:default=true for CloudProviderEnabled and set defaults in defaulting webhook Jan 27, 2025
@Sunnatillo
Copy link
Member

/test ?

@metal3-io-bot
Copy link
Contributor

@Sunnatillo: The following commands are available to trigger required jobs:

/test build
/test generate
/test gomod
/test manifestlint
/test markdownlint
/test metal3-centos-e2e-integration-test-main
/test metal3-ubuntu-e2e-integration-test-main
/test shellcheck
/test test
/test unit

The following commands are available to trigger optional jobs:

/test metal3-centos-e2e-basic-test-main
/test metal3-centos-e2e-feature-test-main-features
/test metal3-centos-e2e-feature-test-main-pivoting
/test metal3-centos-e2e-feature-test-main-remediation
/test metal3-centos-e2e-feature-test-main-scalability
/test metal3-e2e-1-29-1-30-upgrade-test-main
/test metal3-e2e-clusterctl-upgrade-test-main
/test metal3-ubuntu-e2e-basic-test-main
/test metal3-ubuntu-e2e-feature-test-main-features
/test metal3-ubuntu-e2e-feature-test-main-pivoting
/test metal3-ubuntu-e2e-feature-test-main-remediation

Use /test all to run the following jobs that were automatically triggered:

build
generate
gomod
manifestlint
unit

In response to this:

/test ?

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.

Copy link
Member

@Sunnatillo Sunnatillo 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 Jan 27, 2025
@tuminoid
Copy link
Member

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

@tuminoid
Copy link
Member

Actual test need to be done via test PR in dev-env, targeting this branch.

/cc @kashifest
PTAL.

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.

IMO this seems fine.

@elfosardo
Copy link
Member

/approve

@kashifest
Copy link
Member

What this PR does / why we need it: instead of setting // +kubebuilder:default=true for CloudProviderEnabled we are setting defaults in defaulting webhook. This is to avoid conflicts with NoCloudProvider.

It is still not clear to me what issue is it solving, and why the current way of setting the default using kubebuilder tag is an issue, can you describe the issue in a bit more detail ?

@adilGhaffarDev
Copy link
Member Author

It is still not clear to me what issue is it solving, and why the current way of setting the default using kubebuilder tag is an issue, can you describe the issue in a bit more detail ?

i have updated the description of this PR and also the previous PR please check.

@kashifest
Copy link
Member

i have updated the description of this PR and also the previous PR please check.

Thanks a lot. We had offline discussion as well to clear the issue out.

/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elfosardo, 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:

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 Jan 27, 2025
@kashifest
Copy link
Member

/retest

@tuminoid
Copy link
Member

Needs rebase on top of #2277 after that has merged.

Signed-off-by: Muhammad Adil Ghaffar <[email protected]>
@adilGhaffarDev adilGhaffarDev force-pushed the fix-noccloudprovider/adil branch from 005857d to 6ebd717 Compare January 27, 2025 14:39
@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2025
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

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2025
@tuminoid
Copy link
Member

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

@metal3-io-bot metal3-io-bot merged commit 59a0379 into metal3-io:main Jan 27, 2025
14 checks passed
@metal3-io-bot metal3-io-bot added this to the CAPM3 - v1.10 milestone Jan 27, 2025
@metal3-io-bot metal3-io-bot deleted the fix-noccloudprovider/adil branch January 27, 2025 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants