Skip to content

Commit 6f0d732

Browse files
yongxiuYongxiu Cui
yongxiu
authored and
Yongxiu Cui
committed
Fix controller-tools doesn't support single files as input
#837
1 parent 881ffb4 commit 6f0d732

10 files changed

+764
-69
lines changed

‎pkg/loader/loader.go

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -389,12 +389,6 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) ([]*Package, err
389389
}
390390
}()
391391

392-
// uniquePkgIDs is used to keep track of the discovered packages to be nice
393-
// and try and prevent packages from showing up twice when nested module
394-
// support is enabled. there is not harm that comes from this per se, but
395-
// it makes testing easier when a known number of modules can be asserted
396-
uniquePkgIDs := sets.String{}
397-
398392
// loadPackages returns the Go packages for the provided roots
399393
//
400394
// if validatePkgFn is nil, a package will be returned in the slice,
@@ -412,10 +406,7 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) ([]*Package, err
412406
var pkgs []*Package
413407
for _, rp := range rawPkgs {
414408
p := l.packageFor(rp)
415-
if !uniquePkgIDs.Has(p.ID) {
416-
pkgs = append(pkgs, p)
417-
uniquePkgIDs.Insert(p.ID)
418-
}
409+
pkgs = append(pkgs, p)
419410
}
420411
return pkgs, nil
421412
}
@@ -568,13 +559,14 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) ([]*Package, err
568559
for _, r := range fspRoots {
569560
b, d := filepath.Base(r), filepath.Dir(r)
570561

571-
// we want the base part of the path to be either "..." or ".", except
572-
// Go's filepath utilities clean paths during manipulation, removing the
573-
// ".". thus, if not "...", let's update the path components so that:
562+
// we want the base part of the path to be either "..." or ".", except Go's
563+
// filepath utilities clean paths during manipulation or go file path,
564+
// removing the ".". thus, if not "..." or go file, let's update the path
565+
// components so that:
574566
//
575567
// d = r
576568
// b = "."
577-
if b != "..." {
569+
if b != "..." && filepath.Ext(b) != ".go" {
578570
d = r
579571
b = "."
580572
}

‎pkg/loader/loader_test.go

Lines changed: 65 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -33,17 +33,16 @@ var _ = Describe("Loader parsing root module", func() {
3333
testmodPkg = loaderPkg + "/testmod"
3434
)
3535

36-
var indexOfPackage = func(pkgID string, pkgs []*loader.Package) int {
37-
for i := range pkgs {
38-
if pkgs[i].ID == pkgID {
39-
return i
40-
}
41-
}
42-
return -1
36+
var assertPkgExists = func(pkgID string, pkgs map[string]struct{}) {
37+
Expect(pkgs).Should(HaveKey(pkgID))
4338
}
4439

45-
var assertPkgExists = func(pkgID string, pkgs []*loader.Package) {
46-
ExpectWithOffset(1, indexOfPackage(pkgID, pkgs)).Should(BeNumerically(">", -1))
40+
var dedupPkgs = func(pkgs []*loader.Package) map[string]struct{} {
41+
uniquePkgs := make(map[string]struct{})
42+
for _, p := range pkgs {
43+
uniquePkgs[p.ID] = struct{}{}
44+
}
45+
return uniquePkgs
4746
}
4847

4948
Context("with named packages/modules", func() {
@@ -67,37 +66,40 @@ var _ = Describe("Loader parsing root module", func() {
6766
It("should load one package", func() {
6867
pkgs, err := loader.LoadRoots("sigs.k8s.io/controller-tools/pkg/loader/testmod/submod1")
6968
Expect(err).ToNot(HaveOccurred())
70-
Expect(pkgs).To(HaveLen(1))
71-
assertPkgExists(testmodPkg+"/submod1", pkgs)
69+
uniquePkgs := dedupPkgs(pkgs)
70+
Expect(uniquePkgs).To(HaveLen(1))
71+
assertPkgExists(testmodPkg+"/submod1", uniquePkgs)
7272
})
7373
})
7474

7575
Context("with roots=[sigs.k8s.io/controller-tools/pkg/loader/testmod/...]", func() {
7676
It("should load six packages", func() {
7777
pkgs, err := loader.LoadRoots("sigs.k8s.io/controller-tools/pkg/loader/testmod/...")
7878
Expect(err).ToNot(HaveOccurred())
79-
Expect(pkgs).To(HaveLen(6))
80-
assertPkgExists(testmodPkg, pkgs)
81-
assertPkgExists(testmodPkg+"/subdir1", pkgs)
82-
assertPkgExists(testmodPkg+"/subdir1/subdir1", pkgs)
83-
assertPkgExists(testmodPkg+"/subdir1/subdir2", pkgs)
84-
assertPkgExists(testmodPkg+"/submod1", pkgs)
85-
assertPkgExists(testmodPkg+"/submod1/subdir1", pkgs)
79+
uniquePkgs := dedupPkgs(pkgs)
80+
Expect(uniquePkgs).To(HaveLen(6))
81+
assertPkgExists(testmodPkg, uniquePkgs)
82+
assertPkgExists(testmodPkg+"/subdir1", uniquePkgs)
83+
assertPkgExists(testmodPkg+"/subdir1/subdir1", uniquePkgs)
84+
assertPkgExists(testmodPkg+"/subdir1/subdir2", uniquePkgs)
85+
assertPkgExists(testmodPkg+"/submod1", uniquePkgs)
86+
assertPkgExists(testmodPkg+"/submod1/subdir1", uniquePkgs)
8687
})
8788
})
8889

8990
Context("with roots=[sigs.k8s.io/controller-tools/pkg/loader/testmod/..., ./...]", func() {
9091
It("should load seven packages", func() {
9192
pkgs, err := loader.LoadRoots("sigs.k8s.io/controller-tools/pkg/loader/testmod/...", "./...")
9293
Expect(err).ToNot(HaveOccurred())
93-
Expect(pkgs).To(HaveLen(7))
94-
assertPkgExists(testmodPkg, pkgs)
95-
assertPkgExists(testmodPkg+"/subdir1", pkgs)
96-
assertPkgExists(testmodPkg+"/subdir1/subdir1", pkgs)
97-
assertPkgExists(testmodPkg+"/subdir1/subdir2", pkgs)
98-
assertPkgExists(testmodPkg+"/subdir1/submod1", pkgs)
99-
assertPkgExists(testmodPkg+"/submod1", pkgs)
100-
assertPkgExists(testmodPkg+"/submod1/subdir1", pkgs)
94+
uniquePkgs := dedupPkgs(pkgs)
95+
Expect(uniquePkgs).To(HaveLen(7))
96+
assertPkgExists(testmodPkg, uniquePkgs)
97+
assertPkgExists(testmodPkg+"/subdir1", uniquePkgs)
98+
assertPkgExists(testmodPkg+"/subdir1/subdir1", uniquePkgs)
99+
assertPkgExists(testmodPkg+"/subdir1/subdir2", uniquePkgs)
100+
assertPkgExists(testmodPkg+"/subdir1/submod1", uniquePkgs)
101+
assertPkgExists(testmodPkg+"/submod1", uniquePkgs)
102+
assertPkgExists(testmodPkg+"/submod1/subdir1", uniquePkgs)
101103
})
102104
})
103105
})
@@ -106,26 +108,29 @@ var _ = Describe("Loader parsing root module", func() {
106108
It("should load one package", func() {
107109
pkgs, err := loader.LoadRoots("../crd/.")
108110
Expect(err).ToNot(HaveOccurred())
109-
Expect(pkgs).To(HaveLen(1))
110-
assertPkgExists(pkgPkg+"/crd", pkgs)
111+
uniquePkgs := dedupPkgs(pkgs)
112+
Expect(uniquePkgs).To(HaveLen(1))
113+
assertPkgExists(pkgPkg+"/crd", uniquePkgs)
111114
})
112115
})
113116

114117
Context("with roots=[./]", func() {
115118
It("should load one package", func() {
116119
pkgs, err := loader.LoadRoots("./")
117120
Expect(err).ToNot(HaveOccurred())
118-
Expect(pkgs).To(HaveLen(1))
119-
assertPkgExists(loaderPkg, pkgs)
121+
uniquePkgs := dedupPkgs(pkgs)
122+
Expect(uniquePkgs).To(HaveLen(1))
123+
assertPkgExists(loaderPkg, uniquePkgs)
120124
})
121125
})
122126

123127
Context("with roots=[../../pkg/loader]", func() {
124128
It("should load one package", func() {
125129
pkgs, err := loader.LoadRoots("../../pkg/loader")
126130
Expect(err).ToNot(HaveOccurred())
127-
Expect(pkgs).To(HaveLen(1))
128-
assertPkgExists(loaderPkg, pkgs)
131+
uniquePkgs := dedupPkgs(pkgs)
132+
Expect(uniquePkgs).To(HaveLen(1))
133+
assertPkgExists(loaderPkg, uniquePkgs)
129134
})
130135
})
131136

@@ -135,57 +140,62 @@ var _ = Describe("Loader parsing root module", func() {
135140
"../../pkg/loader/../loader/testmod/...",
136141
"./testmod/./../testmod//.")
137142
Expect(err).ToNot(HaveOccurred())
138-
Expect(pkgs).To(HaveLen(7))
139-
assertPkgExists(testmodPkg, pkgs)
140-
assertPkgExists(testmodPkg+"/subdir1", pkgs)
141-
assertPkgExists(testmodPkg+"/subdir1/subdir1", pkgs)
142-
assertPkgExists(testmodPkg+"/subdir1/subdir2", pkgs)
143-
assertPkgExists(testmodPkg+"/subdir1/submod1", pkgs)
144-
assertPkgExists(testmodPkg+"/submod1", pkgs)
145-
assertPkgExists(testmodPkg+"/submod1/subdir1", pkgs)
143+
uniquePkgs := dedupPkgs(pkgs)
144+
Expect(uniquePkgs).To(HaveLen(7))
145+
assertPkgExists(testmodPkg, uniquePkgs)
146+
assertPkgExists(testmodPkg+"/subdir1", uniquePkgs)
147+
assertPkgExists(testmodPkg+"/subdir1/subdir1", uniquePkgs)
148+
assertPkgExists(testmodPkg+"/subdir1/subdir2", uniquePkgs)
149+
assertPkgExists(testmodPkg+"/subdir1/submod1", uniquePkgs)
150+
assertPkgExists(testmodPkg+"/submod1", uniquePkgs)
151+
assertPkgExists(testmodPkg+"/submod1/subdir1", uniquePkgs)
146152
})
147153
})
148154

149155
Context("with roots=[./testmod/...]", func() {
150156
It("should load seven packages", func() {
151157
pkgs, err := loader.LoadRoots("./testmod/...")
152158
Expect(err).ToNot(HaveOccurred())
153-
Expect(pkgs).To(HaveLen(7))
154-
assertPkgExists(testmodPkg, pkgs)
155-
assertPkgExists(testmodPkg+"/subdir1", pkgs)
156-
assertPkgExists(testmodPkg+"/subdir1/subdir1", pkgs)
157-
assertPkgExists(testmodPkg+"/subdir1/subdir2", pkgs)
158-
assertPkgExists(testmodPkg+"/subdir1/submod1", pkgs)
159-
assertPkgExists(testmodPkg+"/submod1", pkgs)
160-
assertPkgExists(testmodPkg+"/submod1/subdir1", pkgs)
159+
uniquePkgs := dedupPkgs(pkgs)
160+
Expect(uniquePkgs).To(HaveLen(7))
161+
assertPkgExists(testmodPkg, uniquePkgs)
162+
assertPkgExists(testmodPkg+"/subdir1", uniquePkgs)
163+
assertPkgExists(testmodPkg+"/subdir1/subdir1", uniquePkgs)
164+
assertPkgExists(testmodPkg+"/subdir1/subdir2", uniquePkgs)
165+
assertPkgExists(testmodPkg+"/subdir1/submod1", uniquePkgs)
166+
assertPkgExists(testmodPkg+"/submod1", uniquePkgs)
167+
assertPkgExists(testmodPkg+"/submod1/subdir1", uniquePkgs)
161168
})
162169
})
163170

164171
Context("with roots=[./testmod/subdir1/submod1/...]", func() {
165172
It("should load one package", func() {
166173
pkgs, err := loader.LoadRoots("./testmod/subdir1/submod1/...")
167174
Expect(err).ToNot(HaveOccurred())
168-
Expect(pkgs).To(HaveLen(1))
169-
assertPkgExists(testmodPkg+"/subdir1/submod1", pkgs)
175+
uniquePkgs := dedupPkgs(pkgs)
176+
Expect(uniquePkgs).To(HaveLen(1))
177+
assertPkgExists(testmodPkg+"/subdir1/submod1", uniquePkgs)
170178
})
171179
})
172180

173181
Context("with roots=[./testmod, ./testmod/submod1]", func() {
174182
It("should load two packages", func() {
175183
pkgs, err := loader.LoadRoots("./testmod", "./testmod/submod1")
176184
Expect(err).ToNot(HaveOccurred())
177-
Expect(pkgs).To(HaveLen(2))
178-
assertPkgExists(testmodPkg, pkgs)
179-
assertPkgExists(testmodPkg+"/submod1", pkgs)
185+
uniquePkgs := dedupPkgs(pkgs)
186+
Expect(uniquePkgs).To(HaveLen(2))
187+
assertPkgExists(testmodPkg, uniquePkgs)
188+
assertPkgExists(testmodPkg+"/submod1", uniquePkgs)
180189
})
181190
})
182191

183192
Context("with roots=[./testmod/submod1/subdir1/]", func() {
184193
It("should load one package", func() {
185194
pkgs, err := loader.LoadRoots("./testmod/submod1/subdir1/")
186195
Expect(err).ToNot(HaveOccurred())
187-
Expect(pkgs).To(HaveLen(1))
188-
assertPkgExists(testmodPkg+"/submod1/subdir1", pkgs)
196+
uniquePkgs := dedupPkgs(pkgs)
197+
Expect(uniquePkgs).To(HaveLen(1))
198+
assertPkgExists(testmodPkg+"/submod1/subdir1", uniquePkgs)
189199
})
190200
})
191201
})

‎pkg/webhook/parser_integration_test.go

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -326,6 +326,141 @@ var _ = Describe("Webhook Generation From Parsing to CustomResourceDefinition",
326326
err = webhook.Generator{}.Generate(genCtx)
327327
Expect(err).To(HaveOccurred())
328328
})
329+
330+
It("should properly generate the webhook definition for single file one", func() {
331+
By("switching into testdata to appease go modules")
332+
cwd, err := os.Getwd()
333+
Expect(err).NotTo(HaveOccurred())
334+
Expect(os.Chdir("./testdata/valid-single")).To(Succeed()) // go modules are directory-sensitive
335+
defer func() { Expect(os.Chdir(cwd)).To(Succeed()) }()
336+
337+
By("loading the golang file")
338+
pkgs, err := loader.LoadRoots("webhook_one.go")
339+
Expect(err).NotTo(HaveOccurred())
340+
Expect(pkgs).To(HaveLen(1))
341+
342+
By("setting up the parser")
343+
reg := &markers.Registry{}
344+
Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
345+
346+
By("requesting that the manifest be generated")
347+
outputDir, err := os.MkdirTemp("", "webhook-integration-test")
348+
Expect(err).NotTo(HaveOccurred())
349+
defer os.RemoveAll(outputDir)
350+
genCtx := &genall.GenerationContext{
351+
Collector: &markers.Collector{Registry: reg},
352+
Roots: pkgs,
353+
OutputRule: genall.OutputToDirectory(outputDir),
354+
}
355+
Expect(webhook.Generator{}.Generate(genCtx)).To(Succeed())
356+
for _, r := range genCtx.Roots {
357+
Expect(r.Errors).To(HaveLen(0))
358+
}
359+
360+
By("loading the generated v1 YAML")
361+
actualFile, err := os.ReadFile(path.Join(outputDir, "manifests.yaml"))
362+
Expect(err).NotTo(HaveOccurred())
363+
actualMutating, actualValidating := unmarshalBothV1(actualFile)
364+
365+
By("loading the desired v1 YAML")
366+
expectedFile, err := os.ReadFile("manifests_one.yaml")
367+
Expect(err).NotTo(HaveOccurred())
368+
expectedMutating, expectedValidating := unmarshalBothV1(expectedFile)
369+
370+
By("comparing the two")
371+
assertSame(actualMutating, expectedMutating)
372+
assertSame(actualValidating, expectedValidating)
373+
})
374+
375+
It("should properly generate the webhook definition for single file two", func() {
376+
By("switching into testdata to appease go modules")
377+
cwd, err := os.Getwd()
378+
Expect(err).NotTo(HaveOccurred())
379+
Expect(os.Chdir("./testdata/valid-single")).To(Succeed()) // go modules are directory-sensitive
380+
defer func() { Expect(os.Chdir(cwd)).To(Succeed()) }()
381+
382+
By("loading the golang file")
383+
pkgs, err := loader.LoadRoots("webhook_two.go")
384+
Expect(err).NotTo(HaveOccurred())
385+
Expect(pkgs).To(HaveLen(1))
386+
387+
By("setting up the parser")
388+
reg := &markers.Registry{}
389+
Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
390+
391+
By("requesting that the manifest be generated")
392+
outputDir, err := os.MkdirTemp("", "webhook-integration-test")
393+
Expect(err).NotTo(HaveOccurred())
394+
defer os.RemoveAll(outputDir)
395+
genCtx := &genall.GenerationContext{
396+
Collector: &markers.Collector{Registry: reg},
397+
Roots: pkgs,
398+
OutputRule: genall.OutputToDirectory(outputDir),
399+
}
400+
Expect(webhook.Generator{}.Generate(genCtx)).To(Succeed())
401+
for _, r := range genCtx.Roots {
402+
Expect(r.Errors).To(HaveLen(0))
403+
}
404+
405+
By("loading the generated v1 YAML")
406+
actualFile, err := os.ReadFile(path.Join(outputDir, "manifests.yaml"))
407+
Expect(err).NotTo(HaveOccurred())
408+
actualMutating, actualValidating := unmarshalBothV1(actualFile)
409+
410+
By("loading the desired v1 YAML")
411+
expectedFile, err := os.ReadFile("manifests_two.yaml")
412+
Expect(err).NotTo(HaveOccurred())
413+
expectedMutating, expectedValidating := unmarshalBothV1(expectedFile)
414+
415+
By("comparing the two")
416+
assertSame(actualMutating, expectedMutating)
417+
assertSame(actualValidating, expectedValidating)
418+
})
419+
420+
It("should properly generate the webhook definition for multiple files", func() {
421+
By("switching into testdata to appease go modules")
422+
cwd, err := os.Getwd()
423+
Expect(err).NotTo(HaveOccurred())
424+
Expect(os.Chdir("./testdata/valid-single")).To(Succeed()) // go modules are directory-sensitive
425+
defer func() { Expect(os.Chdir(cwd)).To(Succeed()) }()
426+
427+
By("loading the roots")
428+
pkgs, err := loader.LoadRoots(".")
429+
Expect(err).NotTo(HaveOccurred())
430+
Expect(pkgs).To(HaveLen(1))
431+
432+
By("setting up the parser")
433+
reg := &markers.Registry{}
434+
Expect(reg.Register(webhook.ConfigDefinition)).To(Succeed())
435+
436+
By("requesting that the manifest be generated")
437+
outputDir, err := os.MkdirTemp("", "webhook-integration-test")
438+
Expect(err).NotTo(HaveOccurred())
439+
defer os.RemoveAll(outputDir)
440+
genCtx := &genall.GenerationContext{
441+
Collector: &markers.Collector{Registry: reg},
442+
Roots: pkgs,
443+
OutputRule: genall.OutputToDirectory(outputDir),
444+
}
445+
Expect(webhook.Generator{}.Generate(genCtx)).To(Succeed())
446+
for _, r := range genCtx.Roots {
447+
Expect(r.Errors).To(HaveLen(0))
448+
}
449+
450+
By("loading the generated v1 YAML")
451+
actualFile, err := os.ReadFile(path.Join(outputDir, "manifests.yaml"))
452+
Expect(err).NotTo(HaveOccurred())
453+
actualMutating, actualValidating := unmarshalBothV1(actualFile)
454+
455+
By("loading the desired v1 YAML")
456+
expectedFile, err := os.ReadFile("manifests_all.yaml")
457+
Expect(err).NotTo(HaveOccurred())
458+
expectedMutating, expectedValidating := unmarshalBothV1(expectedFile)
459+
460+
By("comparing the two")
461+
assertSame(actualMutating, expectedMutating)
462+
assertSame(actualValidating, expectedValidating)
463+
})
329464
})
330465

331466
func unmarshalBothV1(in []byte) (mutating admissionregv1.MutatingWebhookConfiguration, validating admissionregv1.ValidatingWebhookConfiguration) {

0 commit comments

Comments
 (0)