Skip to content
This repository has been archived by the owner on Nov 25, 2024. It is now read-only.

Remove goimports #88

Merged
merged 1 commit into from
Jan 31, 2020
Merged

Remove goimports #88

merged 1 commit into from
Jan 31, 2020

Conversation

negz
Copy link
Member

@negz negz commented Jan 31, 2020

We invoke goimports in two places:

  1. Automatically on generated files.
  2. When running make imports

1 is bad practice; we should not edit generated code, even using tools. 2 is redundant; make lint invokes goimports via golangci-lint.

We invoke goimports in two places:

1. Automatically on generated files.
2. When running make imports

1 is bad practice; we should not edit generated code, even using tools. 2 is
redundant; make lint invokes goimports via golangci-lint.

Signed-off-by: Nic Cope <negz@rk0n.org>
@negz
Copy link
Member Author

negz commented Jan 31, 2020

Some context on this PR: I just lost an hour due to a bug in goimports. Running make generate in stack-azure was deleting corev1 imports from generated deepycopy files, despite them being needed by those files. Turns out make generate invokes goimports on generated files, does not use a pinned version of goimports, and the version of goimports I had cached had a bug in it.

Copy link
Contributor

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jbw976 jbw976 left a comment

Choose a reason for hiding this comment

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

I originally added this in e0558c9 during the migration to kubebuilder v2 because the generated files were failing linting checks (they weren't goimports valid), which was failing the builds. Do we no longer have this problem and the builds will still succeed after this invocation of goimports is removed?

@negz
Copy link
Member Author

negz commented Jan 31, 2020

@jbw976 Was it golangci-lint that was balking, or some other system? golangci-lint should automatically ignore generated files.

@jbw976
Copy link
Member

jbw976 commented Jan 31, 2020

@negz I am not actually sure which specific tool it was (or if it was even linting at all), but a simple make was failing without the goimports call to clean up the generated files. Did you already try running make with this change to the build submodule and it completed OK?

@negz
Copy link
Member Author

negz commented Jan 31, 2020

@jbw976 I tested it with stack-aws, but I just now tried stack-gcp and can reproduce the issue you were seeing. I've confirmed that if I add the following to .golangci-lint.yml there are no issues:

run:
  skip-files:
  - "zz_generated\\..+\\.go$"

My suggestion would be to add the above to the linter config in each repo - I don't think there's much value in linting generated code. I'll open PRs for that now.

@jbw976
Copy link
Member

jbw976 commented Jan 31, 2020

That sounds very reasonable, thank you for checking on that @negz! so we can go ahead and merge this then, and then pick up the new build submodule hash along with updating the .golangci-lint.yml config. Thank you! 💪

@negz
Copy link
Member Author

negz commented Jan 31, 2020

Cross-linking some prior art here:

@negz negz merged commit 510ff11 into upbound:master Jan 31, 2020
@negz negz deleted the noimports branch January 31, 2020 07:58
negz added a commit to negz/provider-gcp that referenced this pull request Jan 31, 2020
upbound/build#88

This results in a diff because we no longer run goimports on generated files.

Signed-off-by: Nic Cope <negz@rk0n.org>
negz added a commit to negz/crossplane-runtime that referenced this pull request Jan 31, 2020
upbound/build#88

This results in a diff because we no longer run goimports on generated files.

Signed-off-by: Nic Cope <negz@rk0n.org>
negz added a commit to negz/provider-azure that referenced this pull request Jan 31, 2020
upbound/build#88

This results in a diff because we no longer run goimports on generated files.

Signed-off-by: Nic Cope <negz@rk0n.org>
negz added a commit to negz/crossplane that referenced this pull request Jan 31, 2020
upbound/build#88

This results in a diff because we no longer run goimports on generated files.

Signed-off-by: Nic Cope <negz@rk0n.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants