Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update CI workflow and drop slices for compatibility #135

Merged
merged 13 commits into from
Nov 21, 2023
Merged

Conversation

oliversun9
Copy link
Contributor

@oliversun9 oliversun9 commented Nov 21, 2023

This PR

  1. Update Go directive in tools/go.mod and go.work to 1.21. This is OK because tools is not a public library.
  2. Update .golangci.yml to exclude gci for results.go, because it has trouble recognizing slices as a standard library (even with Go 1.21)
  3. Update make lint-go and use it in ci.yaml, also add target make lint-go-fix.

# gci is confused by "slices" and suggests to move it to the end of the import list
- path: tools/protovalidate-conformance/internal/results/result.go
linters:
- gci
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without ignoring it, running golangci-lint with --fix would update this file's import list to

    "cmp"
	"github.com/bufbuild/protovalidate/tools/internal/gen/buf/validate"
	"github.com/bufbuild/protovalidate/tools/internal/gen/buf/validate/conformance/harness"
	"slices"

@oliversun9 oliversun9 marked this pull request as draft November 21, 2023 15:43
@oliversun9 oliversun9 marked this pull request as ready for review November 21, 2023 16:12
@nicksnyder
Copy link
Member

All-else-equal, my preference would be to be consistent across all our repos. Right now our template repo and connect-go do not use the golangci-lint-action anymore (preferring instead to simply run make lint in CI). Aside from abstract consistency, this ensures that there is one authoritative version of the version of golangci-lint that we are using.

Would doing it this way here impact CI times at all? (if so, how much)? If no or insignificant impact, can we make this consistent with the other repos?

@oliversun9
Copy link
Contributor Author

oliversun9 commented Nov 21, 2023

All-else-equal, my preference would be to be consistent across all our repos. Right now our template repo and connect-go do not use the golangci-lint-action anymore (preferring instead to simply run make lint in CI). Aside from abstract consistency, this ensures that there is one authoritative version of the version of golangci-lint that we are using.

Would doing it this way here impact CI times at all? (if so, how much)? If no or insignificant impact, can we make this consistent with the other repos?

Using running make lint sounds good.

Here's a past run with make lint, where it needs to download dependencies to build the linter, the lint steps took 44s. In another, this step took 19s.

A run on this PR with the lint action, where the lint step took 24s. In another, it took 22s.

I think the difference is small, 20s in the worst case, and will update the PR.

@oliversun9 oliversun9 merged commit 82d0df3 into main Nov 21, 2023
4 checks passed
@oliversun9 oliversun9 deleted the osun/fix-ci branch November 21, 2023 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants