✨ deepcopy - do not generate empty import#1321
✨ deepcopy - do not generate empty import#1321ivankatliarchuk wants to merge 2 commits intokubernetes-sigs:mainfrom
Conversation
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ivankatliarchuk The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/L
ivankatliarchuk
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/retest |
| By("checking for errors") | ||
| Expect(hadErrs).To(BeFalse()) |
There was a problem hiding this comment.
Why is this so late? Surely you want to check this straight after the Run, else the rest of the test doesn't make a lot of sense?
There was a problem hiding this comment.
I copied same pattern as in other test
. Moved up
pkg/deepcopy/gen_test.go
Outdated
| output := buf.String() | ||
|
|
||
| // Check for empty import block "import ()" | ||
| if strings.Contains(output, "import (\n)") { |
There was a problem hiding this comment.
Would it normally do import () or import( with a newline before the closing )?
There was a problem hiding this comment.
Resolved. As currently it is import ()
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
| 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
Maybe I'm missing something, but why not just check that there is no import in the entire file?
Also we already check that the file is identical to zz_generated.deepcopy.go so this should be easily enough?
sbueringer
changed the title
4 participants
Controller-gen was generating empty
import ()blocks in DeepCopy files when types didn't require external dependenciesModified the writeHeader function to conditionally generate import blocks:
Create unit test Unit Tests.
Added comprehensive E2E test that:
Before (with empty imports):
After (clean output):