Skip to content

Enable GCI linter#17894

Merged
jakule merged 16 commits intomasterfrom
jakule/enable-gci
Oct 28, 2022
Merged

Enable GCI linter#17894
jakule merged 16 commits intomasterfrom
jakule/enable-gci

Conversation

@jakule
Copy link
Copy Markdown
Contributor

@jakule jakule commented Oct 27, 2022

Sort all imports in a deterministic way.
GCI linter generates deterministic Go imports when run with the --run flag.
Introduced make fix-imports to make the process easier.

TODO:

  • Update e ref when e PR is merged

@github-actions github-actions Bot requested review from LKozlowski and r0mant October 27, 2022 20:05
@github-actions github-actions Bot added application-access audit-log Issues related to Teleports Audit Log bpf Used to bugs with bpf and enhanced session recording. database-access Database access related issues and PRs desktop-access kubernetes-access machine-id rdp tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Oct 27, 2022
Copy link
Copy Markdown
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

As said before, I'm happy with any deterministic formatting that machines can do and enforce for us.

Please run make fix-imports on teleport.e/ and update the reference here, otherwise it'll break.

Comment thread .golangci.yml Outdated
Comment thread .golangci.yml Outdated
Comment thread .golangci.yml Outdated
@codingllama
Copy link
Copy Markdown
Contributor

TODO:

[ ] Update e ref when e PR is merged

Update it now?

@jakule jakule force-pushed the jakule/enable-gci branch from 1ceced9 to f658883 Compare October 27, 2022 22:43
Copy link
Copy Markdown
Contributor

@mdwn mdwn left a comment

Choose a reason for hiding this comment

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

I'm in, looks like a great idea!

Copy link
Copy Markdown
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

Couple of general notes:

  • I would consider backporting this to at least all active release branches, otherwise backports will be a pain.
  • Does this change local workflows in any way? I.e. do we need to install any extra Go tools that would auto-sort these imports or do we need to run make fix-imports each time before opening a PR?

@jakule
Copy link
Copy Markdown
Contributor Author

jakule commented Oct 28, 2022

Couple of general notes:

  • I would consider backporting this to at least all active release branches, otherwise backports will be a pain.
    I'll do.
  • Does this change local workflows in any way? I.e. do we need to install any extra Go tools that would auto-sort these imports or do we need to run make fix-imports each time before opening a PR?

@r0mant
Because we all properly sort imports the workflow won't change for anyone. According to @espadolini gopls should works fine. The result generated by gci should be very similar to what goimports -w does.

@codingllama
Copy link
Copy Markdown
Contributor

Couple of general notes:

  • I would consider backporting this to at least all active release branches, otherwise backports will be a pain.
    I'll do.
  • Does this change local workflows in any way? I.e. do we need to install any extra Go tools that would auto-sort these imports or do we need to run make fix-imports each time before opening a PR?

@r0mant Because we all properly sort imports the workflow won't change for anyone. According to @espadolini gopls should works fine. The result generated by gci should be very similar to what goimports -w does.

gopls keeps existing import blocks by default, so it's not much noise. Without any extra configuration you'll likely have to move an import here and there. VSCode/gopls setup should be something like this (caveat, I don't use this):

"gopls": {
    "formatting.local": "github.com/gravitational/teleport",
    "formatting.gofumpt": true, // optional
},

@espadolini
Copy link
Copy Markdown
Contributor

espadolini commented Oct 28, 2022

It gets a little weird with block comments between imports, but single-line comments after the import are fine and as long as the file is already correct with only 3 blocks of imports (standard, nonlocal, local), gopls with those settings should keep it correct according to gci.

@codingllama
Copy link
Copy Markdown
Contributor

If you want to play around with, or just reformat select files, the equivalent invocation to the config is something like this:

gci write -s 'standard,default,prefix(github.com/gravitational/teleport),blank,dot' --skip-generated **/*.go

(Install to GOPATH using go install github.com/daixiang0/gci@latest.)

@jakule jakule force-pushed the jakule/enable-gci branch from 8599d67 to f572b28 Compare October 28, 2022 15:45
Comment thread Makefile Outdated
@jakule jakule enabled auto-merge (squash) October 28, 2022 19:58
@jakule jakule merged commit 0ee91f6 into master Oct 28, 2022
@github-actions
Copy link
Copy Markdown
Contributor

@jakule See the table below for backport results.

Branch Result
branch/v10 Failed
branch/v11 Failed
branch/v9 Failed

jakule added a commit that referenced this pull request Nov 26, 2022
@rosstimothy rosstimothy deleted the jakule/enable-gci branch May 28, 2025 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

application-access audit-log Issues related to Teleports Audit Log bpf Used to bugs with bpf and enhanced session recording. database-access Database access related issues and PRs desktop-access kubernetes-access machine-id rdp tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants