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

upgrade tooling to 2022 standards #78

Merged
merged 14 commits into from
Nov 10, 2022
Merged

upgrade tooling to 2022 standards #78

merged 14 commits into from
Nov 10, 2022

Conversation

majewsky
Copy link
Contributor

@majewsky majewsky commented Nov 1, 2022

@notque This should help you get started. There are a billion little lint errors in make check that you should fix before merging this, but the test suite itself (make build/cover.html) passes.

Note that the Dockerfile uses references to Docker Hub instead of our internal Keppel instance. When you merge this, you want to change your pipeline to chain ci/shared/task-inject-dockerhub-mirror in front of the build job (cf. any of my own pipelines).

Edit: If any of the golangci-lint checks annoy you particularly, you can switch golangciLint.createConfig to false in Makefile.maker.yaml and then turn off the respective checks in the golangci-lint config file. The set that we have active right now is the default selection that we use for most of our services. Most annoying lints can also be resolved by generously sprinkling nolint comments on the codebase wherever justified, see golangci-lint docu for details.

@notque This should help you get started. There are a billion little
lint errors in `make check` that you should fix before merging this,
but the test suite itself (`make build/cover.html`) passes.

Note that the Dockerfile uses references to Docker Hub instead of our
internal Keppel instance. When you merge this, you want to change your
pipeline to chain ci/shared/task-inject-dockerhub-mirror in front of the
build job (cf. any of my own pipelines).
pkg/keystone/keystone.go Fixed Show resolved Hide resolved
pkg/cmd/client.go Fixed Show fixed Hide fixed
@notque
Copy link
Contributor

notque commented Nov 9, 2022

Make check passed, but looks like their are additional ones. Will continue shortly

unsure exactly how to fix the tests, and I would like to get this loaded and running asap. There are many todos i left over, and some ugly code, but getting the rest of this in seems far more important.
@notque
Copy link
Contributor

notque commented Nov 10, 2022

Still erroring, what is the most reasonable way to remove typecheck so that the tests file don't fail on mock drivers. I'd like to get this and hermes merged, and loaded up and running tomorrow in qa so that i can validate everything works, and we get this far. there are obviously lots of todos, and some real ugly lint changes that i need to sort out, but at least to me it seems reasonable enough to move forward with.

The root cause is that code generation did not run as part of
`make static-check`.
@majewsky
Copy link
Contributor Author

what is the most reasonable way to remove typecheck

That's not really possible since typecheck is a prerequisite for all the other checks. The root cause was that code generation was not running as part of make static-check, therefore not all the required code was available to it. I added a dependency from static-check to generate; now it's going through.

@notque notque merged commit 383731e into master Nov 10, 2022
@majewsky majewsky deleted the new-tooling branch November 11, 2022 10:22
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.

2 participants