fix: improve CronJob and multiversion tutorial by refining replaces w…

…ithout overwriting webhook markers Ensure we are more precise with operations to avoid overwriting the default scaffolding. Previously, the markers in the scaffold were unintentionally overwritten when injecting code and documentation, which could override crucial changes. This commit ensures the fix applied in PR #4122 is preserved by refining the tutorial instructions to avoid overwriting webhook markers while still injecting the necessary information. Either we should not export the consts used in the hack/docs since those are internal implementations, therefore the consts used in this area affected have their name changed
kubernetes-sigs · Sep 11, 2024 · 6cdf381 · 6cdf381
1 parent 26c4b55
commit 6cdf381
Show file tree

Hide file tree

Showing 6 changed files with 34 additions and 162 deletions.
diff --git a/docs/book/src/cronjob-tutorial/testdata/project/api/v1/cronjob_webhook.go b/docs/book/src/cronjob-tutorial/testdata/project/api/v1/cronjob_webhook.go
@@ -66,13 +66,8 @@ This marker is responsible for generating a mutating webhook manifest.
 The meaning of each marker can be found [here](/reference/markers/webhook.md).
 */
 
-// +kubebuilder:webhook:path=/mutate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=true,failurePolicy=fail,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=create;update,versions=v1,name=mcronjob.kb.io,sideEffects=None,admissionReviewVersions=v1
-
 /*
-We use the `webhook.CustomDefaulter` interface to set defaults to our CRD.
-A webhook will automatically be served that calls this defaulting.
-
-The `Default` method is expected to mutate the receiver, setting the defaults.
+This marker is responsible for generating a validating webhook manifest.
 */
 
 // +kubebuilder:webhook:path=/mutate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=true,failurePolicy=fail,sideEffects=None,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=create;update,versions=v1,name=mcronjob-v1.kb.io,admissionReviewVersions=v1
@@ -125,12 +120,6 @@ func (r *CronJob) Default() {
 	}
 }
 
-/*
-This marker is responsible for generating a validating webhook manifest.
-*/
-
-// +kubebuilder:webhook:verbs=create;update;delete,path=/validate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=false,failurePolicy=fail,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,versions=v1,name=vcronjob.kb.io,sideEffects=None,admissionReviewVersions=v1
-
 /*
 We can validate our CRD beyond what's possible with declarative
 validation. Generally, declarative validation should be sufficient, but
@@ -153,8 +142,6 @@ Here, however, we just use the same shared validation for `ValidateCreate` and
 validate anything on deletion.
 */
 
-// NOTE: The 'path' attribute must follow a specific pattern and should not be modified directly here.
-// Modifying the path for an invalid path can cause API server errors; failing to locate the webhook.
 // +kubebuilder:webhook:path=/validate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=false,failurePolicy=fail,sideEffects=None,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=create;update,versions=v1,name=vcronjob-v1.kb.io,admissionReviewVersions=v1
 
 // +kubebuilder:object:generate=false

diff --git a/docs/book/src/cronjob-tutorial/testdata/project/config/webhook/manifests.yaml b/docs/book/src/cronjob-tutorial/testdata/project/config/webhook/manifests.yaml
@@ -24,26 +24,6 @@ webhooks:
     resources:
     - cronjobs
   sideEffects: None
-- admissionReviewVersions:
-  - v1
-  clientConfig:
-    service:
-      name: webhook-service
-      namespace: system
-      path: /mutate-batch-tutorial-kubebuilder-io-v1-cronjob
-  failurePolicy: Fail
-  name: mcronjob.kb.io
-  rules:
-  - apiGroups:
-    - batch.tutorial.kubebuilder.io
-    apiVersions:
-    - v1
-    operations:
-    - CREATE
-    - UPDATE
-    resources:
-    - cronjobs
-  sideEffects: None
 ---
 apiVersion: admissionregistration.k8s.io/v1
 kind: ValidatingWebhookConfiguration
@@ -70,24 +50,3 @@ webhooks:
     resources:
     - cronjobs
   sideEffects: None
-- admissionReviewVersions:
-  - v1
-  clientConfig:
-    service:
-      name: webhook-service
-      namespace: system
-      path: /validate-batch-tutorial-kubebuilder-io-v1-cronjob
-  failurePolicy: Fail
-  name: vcronjob.kb.io
-  rules:
-  - apiGroups:
-    - batch.tutorial.kubebuilder.io
-    apiVersions:
-    - v1
-    operations:
-    - CREATE
-    - UPDATE
-    - DELETE
-    resources:
-    - cronjobs
-  sideEffects: None
diff --git a/docs/book/src/multiversion-tutorial/testdata/project/api/v1/cronjob_webhook.go b/docs/book/src/multiversion-tutorial/testdata/project/api/v1/cronjob_webhook.go
@@ -70,13 +70,8 @@ This marker is responsible for generating a mutating webhook manifest.
 The meaning of each marker can be found [here](/reference/markers/webhook.md).
 */
 
-// +kubebuilder:webhook:path=/mutate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=true,failurePolicy=fail,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=create;update,versions=v1,name=mcronjob.kb.io,sideEffects=None,admissionReviewVersions=v1
-
 /*
-We use the `webhook.CustomDefaulter` interface to set defaults to our CRD.
-A webhook will automatically be served that calls this defaulting.
-
-The `Default` method is expected to mutate the receiver, setting the defaults.
+This marker is responsible for generating a validating webhook manifest.
 */
 
 // +kubebuilder:webhook:path=/mutate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=true,failurePolicy=fail,sideEffects=None,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=create;update,versions=v1,name=mcronjob-v1.kb.io,admissionReviewVersions=v1
@@ -129,12 +124,6 @@ func (r *CronJob) Default() {
 	}
 }
 
-/*
-This marker is responsible for generating a validating webhook manifest.
-*/
-
-// +kubebuilder:webhook:verbs=create;update;delete,path=/validate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=false,failurePolicy=fail,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,versions=v1,name=vcronjob.kb.io,sideEffects=None,admissionReviewVersions=v1
-
 /*
 We can validate our CRD beyond what's possible with declarative
 validation. Generally, declarative validation should be sufficient, but
@@ -157,8 +146,6 @@ Here, however, we just use the same shared validation for `ValidateCreate` and
 validate anything on deletion.
 */
 
-// NOTE: The 'path' attribute must follow a specific pattern and should not be modified directly here.
-// Modifying the path for an invalid path can cause API server errors; failing to locate the webhook.
 // +kubebuilder:webhook:path=/validate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=false,failurePolicy=fail,sideEffects=None,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=create;update,versions=v1,name=vcronjob-v1.kb.io,admissionReviewVersions=v1
 
 // +kubebuilder:object:generate=false

diff --git a/docs/book/src/multiversion-tutorial/testdata/project/config/webhook/manifests.yaml b/docs/book/src/multiversion-tutorial/testdata/project/config/webhook/manifests.yaml
@@ -24,26 +24,6 @@ webhooks:
     resources:
     - cronjobs
   sideEffects: None
-- admissionReviewVersions:
-  - v1
-  clientConfig:
-    service:
-      name: webhook-service
-      namespace: system
-      path: /mutate-batch-tutorial-kubebuilder-io-v1-cronjob
-  failurePolicy: Fail
-  name: mcronjob.kb.io
-  rules:
-  - apiGroups:
-    - batch.tutorial.kubebuilder.io
-    apiVersions:
-    - v1
-    operations:
-    - CREATE
-    - UPDATE
-    resources:
-    - cronjobs
-  sideEffects: None
 - admissionReviewVersions:
   - v1
   clientConfig:
@@ -90,27 +70,6 @@ webhooks:
     resources:
     - cronjobs
   sideEffects: None
-- admissionReviewVersions:
-  - v1
-  clientConfig:
-    service:
-      name: webhook-service
-      namespace: system
-      path: /validate-batch-tutorial-kubebuilder-io-v1-cronjob
-  failurePolicy: Fail
-  name: vcronjob.kb.io
-  rules:
-  - apiGroups:
-    - batch.tutorial.kubebuilder.io
-    apiVersions:
-    - v1
-    operations:
-    - CREATE
-    - UPDATE
-    - DELETE
-    resources:
-    - cronjobs
-  sideEffects: None
 - admissionReviewVersions:
   - v1
   clientConfig:

diff --git a/hack/docs/internal/cronjob-tutorial/generate_cronjob.go b/hack/docs/internal/cronjob-tutorial/generate_cronjob.go
@@ -433,7 +433,7 @@ func (sp *Sample) updateWebhook() {
 
 // nolint:unused
 // log is for logging in this package.
-`, WebhookIntro)
+`, webhookIntro)
 	hackutils.CheckError("fixing cronjob_webhook.go", err)
 
 	err = pluginutil.InsertCode(
@@ -447,8 +447,14 @@ Then, we set up the webhook with the manager.
 
 	err = pluginutil.ReplaceInFile(
 		filepath.Join(sp.ctx.Dir, "api/v1/cronjob_webhook.go"),
-		`// TODO(user): EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!`, WebhookMarker)
-	hackutils.CheckError("fixing cronjob_webhook.go by replacing TODO", err)
+		`// TODO(user): EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!`, webhooksNoticeMarker)
+	hackutils.CheckError("fixing cronjob_webhook.go by replacing note about path attribute", err)
+
+	err = pluginutil.ReplaceInFile(
+		filepath.Join(sp.ctx.Dir, "api/v1/cronjob_webhook.go"),
+		`// NOTE: The 'path' attribute must follow a specific pattern and should not be modified directly here.
+// Modifying the path for an invalid path can cause API server errors; failing to locate the webhook.`, explanationValidateCRD)
+	hackutils.CheckError("fixing cronjob_webhook.go by replacing note about path attribute", err)
 
 	err = pluginutil.ReplaceInFile(
 		filepath.Join(sp.ctx.Dir, "api/v1/cronjob_webhook.go"),
@@ -474,48 +480,31 @@ Then, we set up the webhook with the manager.
 	err = pluginutil.ReplaceInFile(
 		filepath.Join(sp.ctx.Dir, "api/v1/cronjob_webhook.go"),
 		`// TODO(user): fill in your defaulting logic.
-`, WebhookDefaultingSettings)
+
+	return nil
+}`, webhookDefaultingSettings)
 	hackutils.CheckError("fixing cronjob_webhook.go by adding logic", err)
 
 	err = pluginutil.ReplaceInFile(
 		filepath.Join(sp.ctx.Dir, "api/v1/cronjob_webhook.go"),
 		`// TODO(user): fill in your validation logic upon object creation.
 
 	return nil, nil`,
-		`
-	return nil, cronjob.validateCronJob()`)
+		`return nil, cronjob.validateCronJob()`)
 	hackutils.CheckError("fixing cronjob_webhook.go by fill in your validation", err)
 
 	err = pluginutil.ReplaceInFile(
 		filepath.Join(sp.ctx.Dir, "api/v1/cronjob_webhook.go"),
 		`// TODO(user): fill in your validation logic upon object update.
 
 	return nil, nil`,
-		`
-	return nil, cronjob.validateCronJob()`)
+		`return nil, cronjob.validateCronJob()`)
 	hackutils.CheckError("fixing cronjob_webhook.go by adding validation logic upon object update", err)
 
-	err = pluginutil.InsertCode(
-		filepath.Join(sp.ctx.Dir, "api/v1/cronjob_webhook.go"),
-		`// TODO(user): fill in your validation logic upon object deletion.
-
-	return nil, nil
-}`, WebhookValidateSpec)
-
-	hackutils.CheckError("fixing cronjob_webhook.go upon object deletion", err)
-
-	err = pluginutil.ReplaceInFile(
+	err = pluginutil.AppendCodeAtTheEnd(
 		filepath.Join(sp.ctx.Dir, "api/v1/cronjob_webhook.go"),
-		`validate anything on deletion.
-*/
-
-	return nil
-}`, `validate anything on deletion.
-*/
-
-`)
-	hackutils.CheckError("fixing cronjob_webhook.go by removing wrong return nil", err)
-
+		webhookValidateSpecMethods)
+	hackutils.CheckError("adding validation spec methods at the end", err)
 }
 
 func (sp *Sample) updateSuiteTest() {

diff --git a/hack/docs/internal/cronjob-tutorial/webhook_implementation.go b/hack/docs/internal/cronjob-tutorial/webhook_implementation.go
@@ -16,7 +16,7 @@ limitations under the License.
 
 package cronjob
 
-const WebhookIntro = `"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+const webhookIntro = `"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
 // +kubebuilder:docs-gen:collapse=Go imports
@@ -27,26 +27,7 @@ Next, we'll setup a logger for the webhooks.
 
 `
 
-const WebhookMarker = `/*
-Notice that we use kubebuilder markers to generate webhook manifests.
-This marker is responsible for generating a mutating webhook manifest.
-
-The meaning of each marker can be found [here](/reference/markers/webhook.md).
-*/
-
-// +kubebuilder:webhook:path=/mutate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=true,failurePolicy=fail,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=create;update,versions=v1,name=mcronjob.kb.io,sideEffects=None,admissionReviewVersions=v1
-
-/*
-We use the` + " `" + `webhook.CustomDefaulter` + "`" + ` interface to set defaults to our CRD.
-A webhook will automatically be served that calls this defaulting.
-
-The` + " `" + `Default` + "`" + ` method is expected to mutate the receiver, setting the defaults.
-*/
-`
-
-const WebhookDefaultingSettings = `
-
-	// Set default values
+const webhookDefaultingSettings = `// Set default values
 	cronjob.Default()
 	
 	return nil
@@ -68,13 +49,22 @@ func (r *CronJob) Default() {
 		*r.Spec.FailedJobsHistoryLimit = 1
 	}
 }
+`
 
+const webhooksNoticeMarker = `
 /*
-This marker is responsible for generating a validating webhook manifest.
+Notice that we use kubebuilder markers to generate webhook manifests.
+This marker is responsible for generating a mutating webhook manifest.
+
+The meaning of each marker can be found [here](/reference/markers/webhook.md).
 */
 
-// +kubebuilder:webhook:verbs=create;update;delete,path=/validate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=false,failurePolicy=fail,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,versions=v1,name=vcronjob.kb.io,sideEffects=None,admissionReviewVersions=v1
+/*
+This marker is responsible for generating a validating webhook manifest.
+*/
+`
 
+const explanationValidateCRD = `
 /*
 We can validate our CRD beyond what's possible with declarative
 validation. Generally, declarative validation should be sufficient, but
@@ -96,8 +86,9 @@ Here, however, we just use the same shared validation for` + " `" + `ValidateCre
 ` + "`" + `ValidateUpdate` + "`" + `. And we do nothing in` + " `" + `ValidateDelete` + "`" + `, since we don't need to
 validate anything on deletion.
 */
+
 `
-const WebhookValidateSpec = `
+const webhookValidateSpecMethods = `
 /*
 We validate the name and the spec of the CronJob.
 */