Skip to content

Conversation

@ivankatliarchuk
Copy link
Member

Controller-gen was generating empty import () blocks in DeepCopy files when types didn't require external dependencies

Modified the writeHeader function to conditionally generate import blocks:

  • Added logic to check if importSpecs has any entries before writing the import block
  • Prevents generation of empty import () statements

Create unit test Unit Tests.

Added comprehensive E2E test that:

  • Generates actual deepcopy code from real Go source files
  • Verifies no empty import blocks exist in output
  • Confirms no import statement appears when unnecessary
  • Compares generated output with expected golden file

Before (with empty imports):

  package simpletypes

  import ()

  // DeepCopyInto is an autogenerated deepcopy function...

After (clean output):

  package simpletypes

  // DeepCopyInto is an autogenerated deepcopy function...

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ivankatliarchuk
Once this PR has been reviewed and has the lgtm label, please assign alvaroaleman for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 4, 2026
@ivankatliarchuk ivankatliarchuk changed the title fix(deepcopy): do not generate empty import Non-breaking feature: ✨ deepcopy - do not generate empty import Jan 4, 2026
@ivankatliarchuk ivankatliarchuk marked this pull request as draft January 4, 2026 16:52
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 4, 2026
@ivankatliarchuk ivankatliarchuk marked this pull request as ready for review January 4, 2026 16:52
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 4, 2026
@ivankatliarchuk
Copy link
Member Author

/retest

Comment on lines 165 to 166
By("checking for errors")
Expect(hadErrs).To(BeFalse())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied same pattern as in other test

Expect(hadErrs).To(BeFalse())
. Moved up

output := buf.String()

// Check for empty import block "import ()"
if strings.Contains(output, "import (\n)") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it normally do import () or import( with a newline before the closing )?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved. As currently it is import ()

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants