diff --git a/pkg/deepcopy/deepcopy_integration_test.go b/pkg/deepcopy/deepcopy_integration_test.go index 798b41023..d633d996c 100644 --- a/pkg/deepcopy/deepcopy_integration_test.go +++ b/pkg/deepcopy/deepcopy_integration_test.go @@ -20,6 +20,7 @@ import ( "io" "os" "slices" + "strings" "github.com/google/go-cmp/cmp" . "github.com/onsi/ginkgo" @@ -104,4 +105,63 @@ var _ = Describe("CRD Generation From Parsing to CustomResourceDefinition", func By("checking for errors") Expect(hadErrs).To(BeFalse()) }) + + It("should not generate empty import blocks for types without external dependencies", func() { + By("switching into simpletypes testdata directory") + cwd, err := os.Getwd() + Expect(err).NotTo(HaveOccurred()) + Expect(os.Chdir("./testdata/simpletypes")).To(Succeed()) + defer func() { Expect(os.Chdir(cwd)).To(Succeed()) }() + + output := make(outputToMap) + + By("initializing the runtime") + optionsRegistry := &markers.Registry{} + Expect(optionsRegistry.Register(markers.Must(markers.MakeDefinition("object", markers.DescribesPackage, deepcopy.Generator{})))).To(Succeed()) + rt, err := genall.FromOptions(optionsRegistry, []string{"object"}) + Expect(err).NotTo(HaveOccurred()) + rt.OutputRules = genall.OutputRules{Default: output} + + By("running the generator and checking for errors") + hadErrs := rt.Run() + By("checking for errors") + Expect(hadErrs).To(BeFalse()) + + By("checking that we got output contents") + Expect(output.fileList()).To(ContainElement("zz_generated.deepcopy.go")) + outFile := output["zz_generated.deepcopy.go"] + Expect(outFile).NotTo(BeNil()) + outContents := string(outFile.contents) + + By("verifying no empty import block exists") + Expect(outContents).NotTo(ContainSubstring("import ()"), "generated code should not contain empty import block") + Expect(outContents).NotTo(ContainSubstring("import (\n)"), "generated code should not contain empty import block") + + By("verifying no import block at all when no imports needed") + lines := strings.Split(outContents, "\n") + for i, line := range lines { + if strings.HasPrefix(line, "package ") { + // The line after package declaration should not be "import (" + if i+1 < len(lines) && i+2 < len(lines) { + // Allow for empty lines between package and first function + nextNonEmptyLine := "" + for j := i + 1; j < len(lines); j++ { + if strings.TrimSpace(lines[j]) != "" { + nextNonEmptyLine = lines[j] + break + } + } + Expect(nextNonEmptyLine).NotTo(HavePrefix("import"), "should not have import block when no imports are needed") + } + break + } + } + + By("loading the expected code") + expectedFile, err := os.ReadFile("zz_generated.deepcopy.go") + Expect(err).NotTo(HaveOccurred()) + + By("comparing the generated output with expected") + Expect(outContents).To(Equal(string(expectedFile)), "generated code not as expected\n\nDiff:\n\n%s", cmp.Diff([]byte(outContents), expectedFile)) + }) }) diff --git a/pkg/deepcopy/gen.go b/pkg/deepcopy/gen.go index df436ec57..fda53c370 100644 --- a/pkg/deepcopy/gen.go +++ b/pkg/deepcopy/gen.go @@ -175,7 +175,7 @@ type ObjectGenCtx struct { // writeHeader writes out the build tag, package declaration, and imports func writeHeader(pkg *loader.Package, out io.Writer, packageName string, imports *importsList, headerText string) { // NB(directxman12): blank line after build tags to distinguish them from comments - _, err := fmt.Fprintf(out, `//go:build !ignore_autogenerated + headerFormat := `//go:build !ignore_autogenerated %[3]s @@ -183,11 +183,17 @@ func writeHeader(pkg *loader.Package, out io.Writer, packageName string, imports package %[1]s -import ( +` + importSpecs := imports.ImportSpecs() + if len(importSpecs) > 0 { + headerFormat += `import ( %[2]s ) -`, packageName, strings.Join(imports.ImportSpecs(), "\n"), headerText) +` + } + + _, err := fmt.Fprintf(out, headerFormat, packageName, strings.Join(importSpecs, "\n"), headerText) if err != nil { pkg.AddError(err) } diff --git a/pkg/deepcopy/gen_test.go b/pkg/deepcopy/gen_test.go new file mode 100644 index 000000000..b04433a53 --- /dev/null +++ b/pkg/deepcopy/gen_test.go @@ -0,0 +1,138 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package deepcopy + +import ( + "bytes" + "strings" + "testing" + "unsafe" + + "golang.org/x/tools/go/packages" + "sigs.k8s.io/controller-tools/pkg/loader" +) + +func TestWriteHeader(t *testing.T) { + // Use unsafe to set the private imports field on the importsList's pkg to avoid needing a loader + type packageHack struct { + *packages.Package + imports map[string]*loader.Package + } + + tests := []struct { + name string + packageName string + imports *importsList + headerText string + wantImport bool + }{ + { + name: "no imports should not generate import block", + packageName: "testpkg", + imports: &importsList{ + byPath: make(map[string]string), + byAlias: make(map[string]string), + pkg: &loader.Package{ + Package: &packages.Package{}, + }, + }, + headerText: "", + }, + { + name: "with imports should generate import block", + packageName: "testpkg", + imports: &importsList{ + byPath: map[string]string{ + "fmt": "fmt", + }, + byAlias: map[string]string{ + "fmt": "fmt", + }, + pkg: &loader.Package{ + Package: &packages.Package{ + Imports: map[string]*packages.Package{ + "fmt": {Name: "fmt"}, + }, + }, + }, + }, + headerText: "", + wantImport: true, + }, + { + name: "no imports with header text", + packageName: "testpkg", + imports: &importsList{ + byPath: make(map[string]string), + byAlias: make(map[string]string), + pkg: &loader.Package{ + Package: &packages.Package{}, + }, + }, + headerText: "// Copyright header\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf bytes.Buffer + pkg := &loader.Package{ + Package: &packages.Package{}, + } + pkgImportsMap := make(map[string]*loader.Package) + // For the with imports test case, populate the imports map + if len(tt.imports.byPath) > 0 { + for path := range tt.imports.byPath { + pkgImportsMap[path] = &loader.Package{ + Package: &packages.Package{Name: tt.imports.byPath[path]}, + } + } + } + (*packageHack)(unsafe.Pointer(tt.imports.pkg)).imports = pkgImportsMap + + writeHeader(pkg, &buf, tt.packageName, tt.imports, tt.headerText) + + output := buf.String() + + // Check for empty import block "import ()" + if strings.Contains(output, "import ()") { + t.Error("generated code contains empty import block 'import ()'") + } + + // Check if import block exists + hasImport := strings.Contains(output, "import (") + if hasImport != tt.wantImport { + t.Errorf("import block presence = %v, want %v\nOutput:\n%s", hasImport, tt.wantImport, output) + } + + // Verify package declaration exists + if !strings.Contains(output, "package "+tt.packageName) { + t.Errorf("missing package declaration, got:\n%s", output) + } + + // Verify build tag exists + if !strings.Contains(output, "//go:build !ignore_autogenerated") { + t.Errorf("missing build tag, got:\n%s", output) + } + + // Verify header text if provided + if tt.headerText != "" && !strings.Contains(output, tt.headerText) { + t.Errorf("missing header text, got:\n%s", output) + } + }) + } +} diff --git a/pkg/deepcopy/testdata/simpletypes/go.mod b/pkg/deepcopy/testdata/simpletypes/go.mod new file mode 100644 index 000000000..cee09252f --- /dev/null +++ b/pkg/deepcopy/testdata/simpletypes/go.mod @@ -0,0 +1,3 @@ +module sigs.k8s.io/controller-tools/pkg/deepcopy/testdata/simpletypes + +go 1.25.0 diff --git a/pkg/deepcopy/testdata/simpletypes/types.go b/pkg/deepcopy/testdata/simpletypes/types.go new file mode 100644 index 000000000..7d9ae7dee --- /dev/null +++ b/pkg/deepcopy/testdata/simpletypes/types.go @@ -0,0 +1,36 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// +kubebuilder:object:generate=true +package simpletypes + +// SimpleType contains only built-in types and should not require any imports +// in the generated deepcopy file. +// +kubebuilder:object:generate=true +type SimpleType struct { + Name string + Values []string + Count int + Data map[string]int +} + +// AnotherSimple also contains only built-in types +// +kubebuilder:object:generate=true +type AnotherSimple struct { + Items map[string]string + Flag bool + Nums []int +} diff --git a/pkg/deepcopy/testdata/simpletypes/zz_generated.deepcopy.go b/pkg/deepcopy/testdata/simpletypes/zz_generated.deepcopy.go new file mode 100644 index 000000000..5e10f2fe4 --- /dev/null +++ b/pkg/deepcopy/testdata/simpletypes/zz_generated.deepcopy.go @@ -0,0 +1,59 @@ +//go:build !ignore_autogenerated + +// Code generated by controller-gen. DO NOT EDIT. + +package simpletypes + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *AnotherSimple) DeepCopyInto(out *AnotherSimple) { + *out = *in + if in.Items != nil { + in, out := &in.Items, &out.Items + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } + if in.Nums != nil { + in, out := &in.Nums, &out.Nums + *out = make([]int, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AnotherSimple. +func (in *AnotherSimple) DeepCopy() *AnotherSimple { + if in == nil { + return nil + } + out := new(AnotherSimple) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *SimpleType) DeepCopyInto(out *SimpleType) { + *out = *in + if in.Values != nil { + in, out := &in.Values, &out.Values + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.Data != nil { + in, out := &in.Data, &out.Data + *out = make(map[string]int, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SimpleType. +func (in *SimpleType) DeepCopy() *SimpleType { + if in == nil { + return nil + } + out := new(SimpleType) + in.DeepCopyInto(out) + return out +}