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

Steve Pipeline, Go Version updates #59

Merged
merged 3 commits into from
Oct 27, 2022

Conversation

MbolotSuse
Copy link
Contributor

This PR aims to add some better CI for steve (test and validate in CI phases), and to update go to version 1.19.

@MbolotSuse MbolotSuse force-pushed the ver-upd-pipeline branch 3 times, most recently from b1f9b1a to 393f0a0 Compare October 14, 2022 15:48
@MbolotSuse
Copy link
Contributor Author

MbolotSuse commented Oct 14, 2022

Overview

Bumps steve's go version to 1.19, adds a pipeline for build/test/lint, and fixes a CI bug causing fossa to be run on every PR.

Changes

  • Bumps the version in go.mod to 1.19
  • Changes the build image used in the CI to use sle-bci golang 1.19
  • Changes the run image used in steve's image to use bci-micro instead of alpine
  • Adds a new build-bin make command which builds the steve binary
  • Adds a new test make command which tests all go files
  • Adds a new validate make command which runs golangci-lint
  • Adds a new .golangci.json file which provides configuration for golangci-lint (mostly copied from rancher/rancher, see notes)
  • Adds a new CI phase which runs the make build-bin command
  • Adds a new CI phase which runs the make test command
  • Adds a new CI phase which runs the make validate command after installing dependencies
  • Fixes small linter issues in a variety of go files which were causing make validate to fail

Notes

  • The golangci-lint config file (.golangci.json) is mostly the same as the one in rancher/rancher. The one difference is that it excludes the lint directive from revive which fails when you have an exported function which stutters - like request.RequestForm (since it has request in package name and in name of the function). I couldn't fix these issues because it would change exported functions, which is a break in behavior and would require a major release. So I turned off the validation for these until we get steve in a place where this is possible.

Copy link
Contributor

@cmurphy cmurphy left a comment

Choose a reason for hiding this comment

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

we will now log a couple of errors that we would have otherwise completely ignored. I think this is ok, but am open to discussing it.

I didn't catch this anywhere in the Go code, could you point it out to me?

scripts/build-bin.sh Outdated Show resolved Hide resolved
scripts/validate.sh Outdated Show resolved Hide resolved
pkg/server/handler/handlers.go Show resolved Hide resolved
@MbolotSuse
Copy link
Contributor Author

I didn't catch this anywhere in the Go code, could you point it out to me?

Sorry, I misremembered. I originally had other linter fixes which didn't make them in (the modified configuration from rancher apparently doesn't look for ignored errors). Since the CI doesn't care about those, I didn't end up pushing them to reduce blast radius of the changes. I'll update my comment to be more accurate - sorry for the misinformation.

@MbolotSuse MbolotSuse force-pushed the ver-upd-pipeline branch 3 times, most recently from a9d6267 to 9480a3e Compare October 14, 2022 20:17
Adds a validate phase to the CI which runs a linter. Also fixes
linter issues discovered during the initial run
}
result <- list.Objects
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Copy link
Contributor

@KevinJoiner KevinJoiner left a comment

Choose a reason for hiding this comment

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

Big fan of the linter changes.

}
return []partition.Partition{
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@MbolotSuse MbolotSuse merged commit 44e5b8d into rancher:master Oct 27, 2022
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