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

Use // +kubebuilder:default annotations where possible #4204

Open
nojnhuh opened this issue Oct 30, 2023 · 9 comments · May be fixed by #5111
Open

Use // +kubebuilder:default annotations where possible #4204

nojnhuh opened this issue Oct 30, 2023 · 9 comments · May be fixed by #5111
Assignees
@bryan-cox
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.

Comments

@nojnhuh
Copy link
Contributor

nojnhuh commented Oct 30, 2023

/kind cleanup
/good-first-issue
/help

What needs cleanup:

There are several places in the defaulting webhooks where CAPZ defaults a static value that could be replaced with the // +kubebuilder:default annotation on the API type.

e.g. this code defaults a static value

cluster-api-provider-azure/api/v1beta1/azuremanagedcontrolplane_webhook.go

Lines 75 to 78 in e2bd592

if m.Spec.NetworkPlugin == nil {
networkPlugin := "azure"
m.Spec.NetworkPlugin = &networkPlugin
}

It should be removed and replaced by the following change to this block:

cluster-api-provider-azure/api/v1beta1/azuremanagedcontrolplane_types.go

Lines 118 to 123 in e2bd592

// NetworkPlugin used for building Kubernetes network.
// Allowed values are "azure", "kubenet".
// Immutable.
// +kubebuilder:validation:Enum=azure;kubenet
// +optional
NetworkPlugin *string `json:"networkPlugin,omitempty"` 

diff --git a/api/v1beta1/azuremanagedcontrolplane_types.go b/api/v1beta1/azuremanagedcontrolplane_types.go
index 5d83db207..35adbf3db 100644
--- a/api/v1beta1/azuremanagedcontrolplane_types.go
+++ b/api/v1beta1/azuremanagedcontrolplane_types.go
@@ -126,6 +126,7 @@ type AzureManagedControlPlaneSpec struct {
 	// Allowed values are "azure", "kubenet".
 	// Immutable.
 	// +kubebuilder:validation:Enum=azure;kubenet
+	// +kubebuilder:default:=azure
 	// +optional
 	NetworkPlugin *string `json:"networkPlugin,omitempty"`

Tests that verify the removed code should also be removed.

After modifying the annotations, run make generate-manifests to encode the new defaults in the CRDs.

Describe the solution you'd like
Walk through the webhooks starting at the following places and replace hand-rolled code to default fields with static fields with the kubebuilder annotation:

cluster-api-provider-azure/api/v1beta1/azuremachine_webhook.go

Line 221 in e2bd592

func (mw *azureMachineWebhook) Default(ctx context.Context, obj runtime.Object) error {

cluster-api-provider-azure/api/v1beta1/azuremachinetemplate_webhook.go

Line 145 in e2bd592

func (r *AzureMachineTemplate) Default(ctx context.Context, obj runtime.Object) error {

cluster-api-provider-azure/api/v1beta1/azuremanagedcontrolplane_webhook.go

Line 70 in e2bd592

func (mw *azureManagedControlPlaneWebhook) Default(ctx context.Context, obj runtime.Object) error {

cluster-api-provider-azure/api/v1beta1/azuremanagedmachinepool_webhook.go

Line 68 in e2bd592

func (mw *azureManagedMachinePoolWebhook) Default(ctx context.Context, obj runtime.Object) error {

cluster-api-provider-azure/exp/api/v1beta1/azuremachinepool_webhook.go

Line 59 in e2bd592

func (ampw *azureMachinePoolWebhook) Default(ctx context.Context, obj runtime.Object) error {

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]
@k8s-ci-robot
Copy link
Contributor

@nojnhuh:
This request has been marked as suitable for new contributors.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/kind cleanup
/good-first-issue
/help

What needs cleanup:

There are several places in the defaulting webhooks where CAPZ defaults a static value that could be replaced with the // +kubebuilder:default annotation on the API type.

e.g. this code defaults a static value

cluster-api-provider-azure/api/v1beta1/azuremanagedcontrolplane_webhook.go

Lines 75 to 78 in e2bd592

if m.Spec.NetworkPlugin == nil {
networkPlugin := "azure"
m.Spec.NetworkPlugin = &networkPlugin
}

It should be removed and replaced by the following change to this block:

cluster-api-provider-azure/api/v1beta1/azuremanagedcontrolplane_types.go

Lines 118 to 123 in e2bd592

// NetworkPlugin used for building Kubernetes network.
// Allowed values are "azure", "kubenet".
// Immutable.
// +kubebuilder:validation:Enum=azure;kubenet
// +optional
NetworkPlugin *string `json:"networkPlugin,omitempty"` 

diff --git a/api/v1beta1/azuremanagedcontrolplane_types.go b/api/v1beta1/azuremanagedcontrolplane_types.go
index 5d83db207..35adbf3db 100644
--- a/api/v1beta1/azuremanagedcontrolplane_types.go
+++ b/api/v1beta1/azuremanagedcontrolplane_types.go
@@ -126,6 +126,7 @@ type AzureManagedControlPlaneSpec struct {
	// Allowed values are "azure", "kubenet".
	// Immutable.
	// +kubebuilder:validation:Enum=azure;kubenet
+	// +kubebuilder:default:=azure
	// +optional
	NetworkPlugin *string `json:"networkPlugin,omitempty"`

Tests that verify the removed code should also be removed.

After modifying the annotations, run make generate-manifests to encode the new defaults in the CRDs.

Describe the solution you'd like
Walk through the webhooks starting at the following places and replace hand-rolled code to default fields with static fields with the kubebuilder annotation:

cluster-api-provider-azure/api/v1beta1/azuremachine_webhook.go

Line 221 in e2bd592

func (mw *azureMachineWebhook) Default(ctx context.Context, obj runtime.Object) error {

cluster-api-provider-azure/api/v1beta1/azuremachinetemplate_webhook.go

Line 145 in e2bd592

func (r *AzureMachineTemplate) Default(ctx context.Context, obj runtime.Object) error {

cluster-api-provider-azure/api/v1beta1/azuremanagedcontrolplane_webhook.go

Line 70 in e2bd592

func (mw *azureManagedControlPlaneWebhook) Default(ctx context.Context, obj runtime.Object) error {

cluster-api-provider-azure/api/v1beta1/azuremanagedmachinepool_webhook.go

Line 68 in e2bd592

func (mw *azureManagedMachinePoolWebhook) Default(ctx context.Context, obj runtime.Object) error {

cluster-api-provider-azure/exp/api/v1beta1/azuremachinepool_webhook.go

Line 59 in e2bd592

func (ampw *azureMachinePoolWebhook) Default(ctx context.Context, obj runtime.Object) error {

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Oct 30, 2023
@k8s-ci-robot k8s-ci-robot added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Oct 30, 2023
@SubhasmitaSw
Copy link

I'd like to work on this, assigning it to myself.

/assign

@SubhasmitaSw SubhasmitaSw mentioned this issue Nov 1, 2023
Closed
@SubhasmitaSw
Copy link

@nojnhuh I've submitted a PR the aforementioned cleanup. Kindly review it at your earliest convenience. Thank you!

@nojnhuh nojnhuh mentioned this issue Nov 2, 2023
Merged
4 tasks
@nojnhuh
Copy link
Contributor Author

nojnhuh commented Dec 14, 2023

@SubhasmitaSw Feel free to re-assign yourself to this issue and reopen #4214 if you plan to get back to this.

/unassign @SubhasmitaSw

@kumarankit999
Copy link

I would like to go with this issue!
If anyone is not working on this issue!

@nojnhuh
Copy link
Contributor Author

nojnhuh commented Jan 8, 2024

Thank you!
/assign @kumarankit999

@bryan-cox
Copy link
Contributor

It doesn't look like there has been any new progress on this issue. I can work on this issue if no one is actively working on it.

@nawazkh
Copy link
Member

nawazkh commented Sep 4, 2024

Feel free to pick it up @bryan-cox !
/unassign @kumarankit999

@bryan-cox
Copy link
Contributor

/assign

@bryan-cox bryan-cox linked a pull request Sep 5, 2024 that will close this issue
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bryan-cox bryan-cox

Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
CAPZ Planning
Status: No status
Milestone
No milestone
6 participants
@nojnhuh @k8s-ci-robot @nawazkh @SubhasmitaSw @bryan-cox @kumarankit999