Skip to content

Commit 764ca45

Browse files
wazerywazery
authored
✨ Improve samples linting & fix samples lint issues
This commit includes: - Fix linter issues - Refactor fetchCronJob in sample controller reconcile func - Fix getting-started tutorial lint issues - Fix multiversion tutorial lint issues - Fix cronjob-tutorial lint issues - Refactor cronjob controller reconcile to reduce cyclomatic complexity
1 parent f9bef1c commit 764ca45

File tree

16 files changed

+100
-47
lines changed

16 files changed

+100
-47
lines changed

‎.github/workflows/lint-sample.yml

+4-1
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,10 @@ jobs:
1616
folder: [
1717
"testdata/project-v4",
1818
"testdata/project-v4-with-plugins",
19-
"testdata/project-v4-multigroup"
19+
"testdata/project-v4-multigroup",
20+
"docs/book/src/cronjob-tutorial/testdata/project",
21+
"docs/book/src/getting-started/testdata/project",
22+
"docs/book/src/multiversion-tutorial/testdata/project"
2023
]
2124
if: (github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository)
2225
steps:

‎docs/book/src/cronjob-tutorial/testdata/project/internal/controller/cronjob_controller.go

+1
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ var (
9696
//
9797
// For more details, check Reconcile and its Result here:
9898
// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.20.1/pkg/reconcile
99+
// nolint:gocyclo
99100
func (r *CronJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
100101
log := log.FromContext(ctx)
101102

‎docs/book/src/cronjob-tutorial/testdata/project/internal/controller/cronjob_controller_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ var _ = Describe("CronJob controller", func() {
129129
By("By checking the CronJob has zero active Jobs")
130130
Consistently(func(g Gomega) {
131131
g.Expect(k8sClient.Get(ctx, cronjobLookupKey, createdCronjob)).To(Succeed())
132-
g.Expect(createdCronjob.Status.Active).To(HaveLen(0))
132+
g.Expect(createdCronjob.Status.Active).To(BeEmpty())
133133
}, duration, interval).Should(Succeed())
134134
/*
135135
Next, we actually create a stubbed Job that will belong to our CronJob, as well as its downstream template specs.

‎docs/book/src/cronjob-tutorial/testdata/project/internal/webhook/v1/cronjob_webhook.go

+1
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ package v1
2020
import (
2121
"context"
2222
"fmt"
23+
2324
"github.com/robfig/cron"
2425
apierrors "k8s.io/apimachinery/pkg/api/errors"
2526
"k8s.io/apimachinery/pkg/runtime/schema"

‎docs/book/src/cronjob-tutorial/testdata/project/internal/webhook/v1/cronjob_webhook_test.go

+13-10
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,13 @@ var _ = Describe("CronJob Webhook", func() {
3232
defaulter CronJobCustomDefaulter
3333
)
3434

35+
const validCronJobName = "valid-cronjob-name"
36+
const schedule = "*/5 * * * *"
37+
3538
BeforeEach(func() {
3639
obj = &batchv1.CronJob{
3740
Spec: batchv1.CronJobSpec{
38-
Schedule: "*/5 * * * *",
41+
Schedule: schedule,
3942
ConcurrencyPolicy: batchv1.AllowConcurrent,
4043
SuccessfulJobsHistoryLimit: new(int32),
4144
FailedJobsHistoryLimit: new(int32),
@@ -46,7 +49,7 @@ var _ = Describe("CronJob Webhook", func() {
4649

4750
oldObj = &batchv1.CronJob{
4851
Spec: batchv1.CronJobSpec{
49-
Schedule: "*/5 * * * *",
52+
Schedule: schedule,
5053
ConcurrencyPolicy: batchv1.AllowConcurrent,
5154
SuccessfulJobsHistoryLimit: new(int32),
5255
FailedJobsHistoryLimit: new(int32),
@@ -80,7 +83,7 @@ var _ = Describe("CronJob Webhook", func() {
8083
obj.Spec.FailedJobsHistoryLimit = nil // This should default to 1
8184

8285
By("calling the Default method to apply defaults")
83-
defaulter.Default(ctx, obj)
86+
_ = defaulter.Default(ctx, obj)
8487

8588
By("checking that the default values are set")
8689
Expect(obj.Spec.ConcurrencyPolicy).To(Equal(batchv1.AllowConcurrent), "Expected ConcurrencyPolicy to default to AllowConcurrent")
@@ -100,7 +103,7 @@ var _ = Describe("CronJob Webhook", func() {
100103
*obj.Spec.FailedJobsHistoryLimit = 2
101104

102105
By("calling the Default method to apply defaults")
103-
defaulter.Default(ctx, obj)
106+
_ = defaulter.Default(ctx, obj)
104107

105108
By("checking that the fields were not overwritten")
106109
Expect(obj.Spec.ConcurrencyPolicy).To(Equal(batchv1.ForbidConcurrent), "Expected ConcurrencyPolicy to retain its set value")
@@ -119,7 +122,7 @@ var _ = Describe("CronJob Webhook", func() {
119122
})
120123

121124
It("Should admit creation if the name is valid", func() {
122-
obj.ObjectMeta.Name = "valid-cronjob-name"
125+
obj.ObjectMeta.Name = validCronJobName
123126
Expect(validator.ValidateCreate(ctx, obj)).To(BeNil(),
124127
"Expected name validation to pass for a valid name")
125128
})
@@ -132,14 +135,14 @@ var _ = Describe("CronJob Webhook", func() {
132135
})
133136

134137
It("Should admit creation if the schedule is valid", func() {
135-
obj.Spec.Schedule = "*/5 * * * *"
138+
obj.Spec.Schedule = schedule
136139
Expect(validator.ValidateCreate(ctx, obj)).To(BeNil(),
137140
"Expected spec validation to pass for a valid schedule")
138141
})
139142

140143
It("Should deny update if both name and spec are invalid", func() {
141-
oldObj.ObjectMeta.Name = "valid-cronjob-name"
142-
oldObj.Spec.Schedule = "*/5 * * * *"
144+
oldObj.ObjectMeta.Name = validCronJobName
145+
oldObj.Spec.Schedule = schedule
143146

144147
By("simulating an update")
145148
obj.ObjectMeta.Name = "this-name-is-way-too-long-and-should-fail-validation-because-it-is-way-too-long"
@@ -151,8 +154,8 @@ var _ = Describe("CronJob Webhook", func() {
151154
})
152155

153156
It("Should admit update if both name and spec are valid", func() {
154-
oldObj.ObjectMeta.Name = "valid-cronjob-name"
155-
oldObj.Spec.Schedule = "*/5 * * * *"
157+
oldObj.ObjectMeta.Name = validCronJobName
158+
oldObj.Spec.Schedule = schedule
156159

157160
By("simulating an update")
158161
obj.ObjectMeta.Name = "valid-cronjob-name-updated"

‎docs/book/src/getting-started/testdata/project/internal/controller/memcached_controller.go

+3-2
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,15 @@ package controller
1919
import (
2020
"context"
2121
"fmt"
22+
"time"
23+
2224
appsv1 "k8s.io/api/apps/v1"
2325
corev1 "k8s.io/api/core/v1"
2426
apierrors "k8s.io/apimachinery/pkg/api/errors"
2527
"k8s.io/apimachinery/pkg/api/meta"
2628
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2729
"k8s.io/apimachinery/pkg/types"
2830
"k8s.io/utils/ptr"
29-
"time"
3031

3132
"k8s.io/apimachinery/pkg/runtime"
3233
ctrl "sigs.k8s.io/controller-runtime"
@@ -89,7 +90,7 @@ func (r *MemcachedReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
8990
}
9091

9192
// Let's just set the status as Unknown when no status is available
92-
if memcached.Status.Conditions == nil || len(memcached.Status.Conditions) == 0 {
93+
if len(memcached.Status.Conditions) == 0 {
9394
meta.SetStatusCondition(&memcached.Status.Conditions, metav1.Condition{Type: typeAvailableMemcached, Status: metav1.ConditionUnknown, Reason: "Reconciling", Message: "Starting reconciliation"})
9495
if err = r.Status().Update(ctx, memcached); err != nil {
9596
log.Error(err, "Failed to update Memcached status")

‎docs/book/src/multiversion-tutorial/testdata/project/internal/controller/cronjob_controller.go

+1
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ var (
9696
//
9797
// For more details, check Reconcile and its Result here:
9898
// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.20.1/pkg/reconcile
99+
// nolint:gocyclo
99100
func (r *CronJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
100101
log := log.FromContext(ctx)
101102

‎docs/book/src/multiversion-tutorial/testdata/project/internal/controller/cronjob_controller_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ var _ = Describe("CronJob controller", func() {
129129
By("By checking the CronJob has zero active Jobs")
130130
Consistently(func(g Gomega) {
131131
g.Expect(k8sClient.Get(ctx, cronjobLookupKey, createdCronjob)).To(Succeed())
132-
g.Expect(createdCronjob.Status.Active).To(HaveLen(0))
132+
g.Expect(createdCronjob.Status.Active).To(BeEmpty())
133133
}, duration, interval).Should(Succeed())
134134
/*
135135
Next, we actually create a stubbed Job that will belong to our CronJob, as well as its downstream template specs.

‎docs/book/src/multiversion-tutorial/testdata/project/internal/webhook/v1/cronjob_webhook.go

+1
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ package v1
2020
import (
2121
"context"
2222
"fmt"
23+
2324
"github.com/robfig/cron"
2425
apierrors "k8s.io/apimachinery/pkg/api/errors"
2526
"k8s.io/apimachinery/pkg/runtime/schema"

‎docs/book/src/multiversion-tutorial/testdata/project/internal/webhook/v1/cronjob_webhook_test.go

+12-10
Original file line numberDiff line numberDiff line change
@@ -31,11 +31,13 @@ var _ = Describe("CronJob Webhook", func() {
3131
validator CronJobCustomValidator
3232
defaulter CronJobCustomDefaulter
3333
)
34+
const validCronJobName = "valid-cronjob-name"
35+
const schedule = "*/5 * * * *"
3436

3537
BeforeEach(func() {
3638
obj = &batchv1.CronJob{
3739
Spec: batchv1.CronJobSpec{
38-
Schedule: "*/5 * * * *",
40+
Schedule: schedule,
3941
ConcurrencyPolicy: batchv1.AllowConcurrent,
4042
SuccessfulJobsHistoryLimit: new(int32),
4143
FailedJobsHistoryLimit: new(int32),
@@ -46,7 +48,7 @@ var _ = Describe("CronJob Webhook", func() {
4648

4749
oldObj = &batchv1.CronJob{
4850
Spec: batchv1.CronJobSpec{
49-
Schedule: "*/5 * * * *",
51+
Schedule: schedule,
5052
ConcurrencyPolicy: batchv1.AllowConcurrent,
5153
SuccessfulJobsHistoryLimit: new(int32),
5254
FailedJobsHistoryLimit: new(int32),
@@ -80,7 +82,7 @@ var _ = Describe("CronJob Webhook", func() {
8082
obj.Spec.FailedJobsHistoryLimit = nil // This should default to 1
8183

8284
By("calling the Default method to apply defaults")
83-
defaulter.Default(ctx, obj)
85+
_ = defaulter.Default(ctx, obj)
8486

8587
By("checking that the default values are set")
8688
Expect(obj.Spec.ConcurrencyPolicy).To(Equal(batchv1.AllowConcurrent), "Expected ConcurrencyPolicy to default to AllowConcurrent")
@@ -100,7 +102,7 @@ var _ = Describe("CronJob Webhook", func() {
100102
*obj.Spec.FailedJobsHistoryLimit = 2
101103

102104
By("calling the Default method to apply defaults")
103-
defaulter.Default(ctx, obj)
105+
_ = defaulter.Default(ctx, obj)
104106

105107
By("checking that the fields were not overwritten")
106108
Expect(obj.Spec.ConcurrencyPolicy).To(Equal(batchv1.ForbidConcurrent), "Expected ConcurrencyPolicy to retain its set value")
@@ -119,7 +121,7 @@ var _ = Describe("CronJob Webhook", func() {
119121
})
120122

121123
It("Should admit creation if the name is valid", func() {
122-
obj.ObjectMeta.Name = "valid-cronjob-name"
124+
obj.ObjectMeta.Name = validCronJobName
123125
Expect(validator.ValidateCreate(ctx, obj)).To(BeNil(),
124126
"Expected name validation to pass for a valid name")
125127
})
@@ -132,14 +134,14 @@ var _ = Describe("CronJob Webhook", func() {
132134
})
133135

134136
It("Should admit creation if the schedule is valid", func() {
135-
obj.Spec.Schedule = "*/5 * * * *"
137+
obj.Spec.Schedule = schedule
136138
Expect(validator.ValidateCreate(ctx, obj)).To(BeNil(),
137139
"Expected spec validation to pass for a valid schedule")
138140
})
139141

140142
It("Should deny update if both name and spec are invalid", func() {
141-
oldObj.ObjectMeta.Name = "valid-cronjob-name"
142-
oldObj.Spec.Schedule = "*/5 * * * *"
143+
oldObj.ObjectMeta.Name = validCronJobName
144+
oldObj.Spec.Schedule = schedule
143145

144146
By("simulating an update")
145147
obj.ObjectMeta.Name = "this-name-is-way-too-long-and-should-fail-validation-because-it-is-way-too-long"
@@ -151,8 +153,8 @@ var _ = Describe("CronJob Webhook", func() {
151153
})
152154

153155
It("Should admit update if both name and spec are valid", func() {
154-
oldObj.ObjectMeta.Name = "valid-cronjob-name"
155-
oldObj.Spec.Schedule = "*/5 * * * *"
156+
oldObj.ObjectMeta.Name = validCronJobName
157+
oldObj.Spec.Schedule = schedule
156158

157159
By("simulating an update")
158160
obj.ObjectMeta.Name = "valid-cronjob-name-updated"

‎hack/docs/internal/cronjob-tutorial/controller_implementation.go

+3
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,9 @@ var (
8484
)
8585
`
8686

87+
const skipGoCycloLint = `
88+
// nolint:gocyclo`
89+
8790
const controllerReconcileLogic = `log := log.FromContext(ctx)
8891
8992
/*

‎hack/docs/internal/cronjob-tutorial/generate_cronjob.go

+15
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,11 @@ func (sp *Sample) updateController() {
286286
`// +kubebuilder:rbac:groups=batch.tutorial.kubebuilder.io,resources=cronjobs/finalizers,verbs=update`, controllerReconcile)
287287
hackutils.CheckError("fixing cronjob_controller.go", err)
288288

289+
err = pluginutil.InsertCode(
290+
filepath.Join(sp.ctx.Dir, "internal/controller/cronjob_controller.go"),
291+
`// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.20.1/pkg/reconcile`, skipGoCycloLint)
292+
hackutils.CheckError("fixing cronjob_controller.go", err)
293+
289294
err = pluginutil.ReplaceInFile(
290295
filepath.Join(sp.ctx.Dir, "internal/controller/cronjob_controller.go"),
291296
` _ = log.FromContext(ctx)
@@ -398,6 +403,11 @@ func (sp *Sample) updateWebhookTests() {
398403
webhookTestingValidatingExampleFragment)
399404
hackutils.CheckError("replace validating defaulting test", err)
400405

406+
err = pluginutil.ReplaceInFile(file,
407+
webhookTestsVars,
408+
webhookTestsConstants)
409+
hackutils.CheckError("replace before each webhook test ", err)
410+
401411
err = pluginutil.ReplaceInFile(file,
402412
webhookTestsBeforeEachOriginal,
403413
webhookTestsBeforeEachChanged)
@@ -513,6 +523,11 @@ Then, we set up the webhook with the manager.
513523
filepath.Join(sp.ctx.Dir, "internal/webhook/v1/cronjob_webhook.go"),
514524
webhookValidateSpecMethods)
515525
hackutils.CheckError("adding validation spec methods at the end", err)
526+
527+
// Run goimports to fix formatting
528+
cmd := exec.Command("goimports", "-w", filepath.Join(sp.ctx.Dir, "internal/webhook/v1/cronjob_webhook.go"))
529+
err = cmd.Run()
530+
hackutils.CheckError("format cronjob_webhook.go with goimports", err)
516531
}
517532

518533
func (sp *Sample) updateSuiteTest() {

‎hack/docs/internal/cronjob-tutorial/webhook_implementation.go

+25-10
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ const webhookTestCreateDefaultingReplaceFragment = `It("Should apply defaults wh
205205
obj.Spec.FailedJobsHistoryLimit = nil // This should default to 1
206206
207207
By("calling the Default method to apply defaults")
208-
defaulter.Default(ctx, obj)
208+
_ = defaulter.Default(ctx, obj)
209209
210210
By("checking that the default values are set")
211211
Expect(obj.Spec.ConcurrencyPolicy).To(Equal(batchv1.AllowConcurrent), "Expected ConcurrencyPolicy to default to AllowConcurrent")
@@ -225,7 +225,7 @@ const webhookTestCreateDefaultingReplaceFragment = `It("Should apply defaults wh
225225
*obj.Spec.FailedJobsHistoryLimit = 2
226226
227227
By("calling the Default method to apply defaults")
228-
defaulter.Default(ctx, obj)
228+
_ = defaulter.Default(ctx, obj)
229229
230230
By("checking that the fields were not overwritten")
231231
Expect(obj.Spec.ConcurrencyPolicy).To(Equal(batchv1.ForbidConcurrent), "Expected ConcurrencyPolicy to retain its set value")
@@ -263,7 +263,7 @@ const webhookTestingValidatingExampleFragment = `It("Should deny creation if the
263263
})
264264
265265
It("Should admit creation if the name is valid", func() {
266-
obj.ObjectMeta.Name = "valid-cronjob-name"
266+
obj.ObjectMeta.Name = validCronJobName
267267
Expect(validator.ValidateCreate(ctx, obj)).To(BeNil(),
268268
"Expected name validation to pass for a valid name")
269269
})
@@ -276,14 +276,14 @@ const webhookTestingValidatingExampleFragment = `It("Should deny creation if the
276276
})
277277
278278
It("Should admit creation if the schedule is valid", func() {
279-
obj.Spec.Schedule = "*/5 * * * *"
279+
obj.Spec.Schedule = schedule
280280
Expect(validator.ValidateCreate(ctx, obj)).To(BeNil(),
281281
"Expected spec validation to pass for a valid schedule")
282282
})
283283
284284
It("Should deny update if both name and spec are invalid", func() {
285-
oldObj.ObjectMeta.Name = "valid-cronjob-name"
286-
oldObj.Spec.Schedule = "*/5 * * * *"
285+
oldObj.ObjectMeta.Name = validCronJobName
286+
oldObj.Spec.Schedule = schedule
287287
288288
By("simulating an update")
289289
obj.ObjectMeta.Name = "this-name-is-way-too-long-and-should-fail-validation-because-it-is-way-too-long"
@@ -295,8 +295,8 @@ const webhookTestingValidatingExampleFragment = `It("Should deny creation if the
295295
})
296296
297297
It("Should admit update if both name and spec are valid", func() {
298-
oldObj.ObjectMeta.Name = "valid-cronjob-name"
299-
oldObj.Spec.Schedule = "*/5 * * * *"
298+
oldObj.ObjectMeta.Name = validCronJobName
299+
oldObj.Spec.Schedule = schedule
300300
301301
By("simulating an update")
302302
obj.ObjectMeta.Name = "valid-cronjob-name-updated"
@@ -307,6 +307,21 @@ const webhookTestingValidatingExampleFragment = `It("Should deny creation if the
307307
"Expected validation to pass for a valid update")
308308
})`
309309

310+
const webhookTestsVars = `var (
311+
obj *batchv1.CronJob
312+
oldObj *batchv1.CronJob
313+
validator CronJobCustomValidator
314+
defaulter CronJobCustomDefaulter
315+
)`
316+
const webhookTestsConstants = ` var (
317+
obj *batchv1.CronJob
318+
oldObj *batchv1.CronJob
319+
validator CronJobCustomValidator
320+
defaulter CronJobCustomDefaulter
321+
)
322+
323+
const validCronJobName = "valid-cronjob-name"
324+
const schedule = "*/5 * * * *"`
310325
const webhookTestsBeforeEachOriginal = `obj = &batchv1.CronJob{}
311326
oldObj = &batchv1.CronJob{}
312327
validator = CronJobCustomValidator{}
@@ -319,7 +334,7 @@ const webhookTestsBeforeEachOriginal = `obj = &batchv1.CronJob{}
319334

320335
const webhookTestsBeforeEachChanged = `obj = &batchv1.CronJob{
321336
Spec: batchv1.CronJobSpec{
322-
Schedule: "*/5 * * * *",
337+
Schedule: schedule,
323338
ConcurrencyPolicy: batchv1.AllowConcurrent,
324339
SuccessfulJobsHistoryLimit: new(int32),
325340
FailedJobsHistoryLimit: new(int32),
@@ -330,7 +345,7 @@ const webhookTestsBeforeEachChanged = `obj = &batchv1.CronJob{
330345
331346
oldObj = &batchv1.CronJob{
332347
Spec: batchv1.CronJobSpec{
333-
Schedule: "*/5 * * * *",
348+
Schedule: schedule,
334349
ConcurrencyPolicy: batchv1.AllowConcurrent,
335350
SuccessfulJobsHistoryLimit: new(int32),
336351
FailedJobsHistoryLimit: new(int32),

‎hack/docs/internal/cronjob-tutorial/writing_tests_controller.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ var _ = Describe("CronJob controller", func() {
148148
By("By checking the CronJob has zero active Jobs")
149149
Consistently(func(g Gomega) {
150150
g.Expect(k8sClient.Get(ctx, cronjobLookupKey, createdCronjob)).To(Succeed())
151-
g.Expect(createdCronjob.Status.Active).To(HaveLen(0))
151+
g.Expect(createdCronjob.Status.Active).To(BeEmpty())
152152
}, duration, interval).Should(Succeed())
153153
/*
154154
Next, we actually create a stubbed Job that will belong to our CronJob, as well as its downstream template specs.

0 commit comments

Comments
 (0)