Skip to content

Run GCI as make fix-imports#17956

Merged
jakule merged 9 commits intomasterfrom
jakule/gci-docker
Nov 4, 2022
Merged

Run GCI as make fix-imports#17956
jakule merged 9 commits intomasterfrom
jakule/gci-docker

Conversation

@jakule
Copy link
Copy Markdown
Contributor

@jakule jakule commented Oct 31, 2022

After #17894 has been merged, we discovered a few issues with using golanglint-ci as a tool to run GCI:

  • Depending on the golanglint-ci, the output may differ
  • Some files are omitted as golanglint-ci require all build tags to be provided, and some combinations (bpf on Mac) are not supported
  • System depended files are not being formatted - golanglint-ci only includes the files that match GOOS
  • nit: examples/ directory is always skipped
  • nit: It is slow

Switching to gci should solve all the above problems. The only con of this solution is the necessity of installing gci on your system. To avoid that, I've added it to the buildbox, so it can be triggered from there.

@github-actions github-actions Bot added audit-log Issues related to Teleports Audit Log bpf Used to bugs with bpf and enhanced session recording. desktop-access rdp labels Oct 31, 2022
@github-actions github-actions Bot requested review from atburke and zmb3 October 31, 2022 15:52
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.

Thanks, Jakub!

Comment thread Makefile
Comment thread Makefile Outdated
Comment thread examples/go-client/main.go
Copy link
Copy Markdown
Contributor

@strideynet strideynet 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 happy w/ this beyond points already raised by @codingllama

@jakule jakule force-pushed the jakule/gci-docker branch from 6ba28aa to 1077c27 Compare November 2, 2022 17:06
@jakule jakule requested a review from codingllama November 2, 2022 17:35
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.

Thanks!

Comment thread Makefile Outdated
Comment thread Makefile Outdated
Comment thread Makefile
echo "gci is not installed or it's missing from PATH, consider installing it 'go install github.com/daixiang0/gci@latest' or use 'make -C build.assets/ fix-imports'";\
exit 1;\
fi
gci write -s 'standard,default,prefix(github.com/gravitational/teleport)' --skip-generated .
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice, I didn't realize gci write . worked.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

**/*.go doesn't work in Makefile as this pattern goes only one level deep (the same as */*.go) It's super confusing as gci runs without an error but doesn't do much.

@github-actions github-actions Bot removed request for atburke and zmb3 November 3, 2022 14:31
jakule and others added 2 commits November 3, 2022 11:14
@jakule jakule enabled auto-merge (squash) November 3, 2022 15:15
@jakule jakule merged commit bea2e89 into master Nov 4, 2022
@zmb3 zmb3 deleted the jakule/gci-docker branch May 7, 2024 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

audit-log Issues related to Teleports Audit Log bpf Used to bugs with bpf and enhanced session recording. desktop-access rdp

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants