Update to go1.17, and bring up linter, and deps #209
Conversation
bnevis-i
requested review from
amartinezfayo,
evan2645 and
azdagron
as code owners
|
Oh, already filed #206 :) |
Github action isn't using the same Go version that is specified in the Makefile. |
v2/go.mod
Outdated
| @@ -3,11 +3,11 @@ module github.com/spiffe/go-spiffe/v2 | |||
| go 1.13 | |||
There was a problem hiding this comment.
update version to 1.18 here too
What are the supported go versions? |
|
I think we should move to go1.17. That is the version that most of our dependencies use as a minimum. |
|
Users are sometimes constrained and can't immediately move to new versions (supported or not). 1.13 is obviously way too old at this point. |
|
Stand by. 1.17 version coming. |
|
what compat mode do you want for the go tidy? |
v2/go.mod
Outdated
| @@ -1,13 +1,27 @@ | |||
| module github.com/spiffe/go-spiffe/v2 | |||
|
|
|||
| go 1.13 | |||
| go 1.18 | |||
There was a problem hiding this comment.
I think we should use go1.17. We should also update the workflow version to match for now until we get a version matrix.
|
Just pushed a version tidied with -go=1.16 AND -go=1.17. Let me know if you want -compat=1.17 |
|
I think running with |
|
We just need to update the .github/workflows/pr_build.yaml to use go1.17 instead of go1.13. |
Resolves CVE-2022-41717 and CVE-2022-32149 and general housekeeping Signed-off-by: Bryon Nevis <[email protected]>
Amended GO_VERSION: 1.17 |
|
I think this PR needs to be put on hold in order to deal with linter issues. Lots of issues upgrading the linter :-( |
|
Yikes. Noticed that. Is that something you are planning to work on @bnevis-i? Or should one of us pick it up? |
I can give it a shot. Unfortunately, a good number of the findings require subjective decisions, like what is the min tls version, how much ReadHeaderTimeout is needed. I can put up a draft PR and get feedback on the right values to use, or I can just wait for someone on the project who knows what the right values are to take care of it. Which would you like? |
|
I can toss up a a PR real quick |
|
Actually, i'll just push a commit on top of this PR. Then maybe we can rename it something more inclusive of the actual changes we're making. |
Signed-off-by: Andrew Harding <[email protected]>
azdagron
changed the title
Signed-off-by: Andrew Harding <[email protected]>
Signed-off-by: Andrew Harding <[email protected]>
|
@azdagron Nice job. I've got to learn to push onto someone else's PR someday. Neat trick. It was the right call to resolve the linters yourself. I would have done every one differently! |
It's not terribly hard. You have to add a new local remote to the forked repo, fetch, and then you can just push onto remote PR branch. You can delete the remote afterwards. You of course have to be a codeowner of the repo that is forked, AND it has to be enabled on the PR by the author (which default is controlled by the user? or maybe the org? I don't recall). |
Resolves CVE-2022-41717 and CVE-2022-32149 and general housekeeping
Signed-off-by: Bryon Nevis [email protected]