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

Mass-update dependencies #42

Merged
merged 1 commit into from
Jun 10, 2024
Merged

Mass-update dependencies #42

merged 1 commit into from
Jun 10, 2024

Conversation

dtantsur
Copy link
Member

@dtantsur dtantsur commented Jun 5, 2024

We're about to pick up active development here again, let's make sure
to work with the latest stuff to avoid accidentally depending on old and
incompatible versions (looking at you, controller-runtime).

Forces bumping the Go version to 1.22 (using 1.22.2 to avoid reverting
the fix for https://osv.dev/vulnerability/GO-2024-2687).

@dtantsur dtantsur force-pushed the deps branch 2 times, most recently from 130e6a6 to 12418dc Compare June 5, 2024 14:01
@dtantsur
Copy link
Member Author

dtantsur commented Jun 5, 2024

/assign @tuminoid

Could you make sure I'm not undoing any of your security fixes?

@dtantsur dtantsur requested a review from tuminoid June 5, 2024 14:24
@tuminoid
Copy link
Member

tuminoid commented Jun 6, 2024

/assign @tuminoid

Could you make sure I'm not undoing any of your security fixes?

You should be bumping to Go 1.22.3, 1.22.2 has 3 vulnerabilities open. Otherwise the tree is clean. 👍

Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

At least Dockerfile and Makefile versions should be aligned?

Dockerfile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@dtantsur
Copy link
Member Author

dtantsur commented Jun 7, 2024

@tuminoid done as you suggested

@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 7, 2024
@dtantsur dtantsur requested a review from tuminoid June 7, 2024 13:57
@dtantsur
Copy link
Member Author

dtantsur commented Jun 9, 2024

@tuminoid looks like github actions only have 1.22.2 on their ubuntu runners :(

@tuminoid
Copy link
Member

@tuminoid looks like github actions only have 1.22.2 on their ubuntu runners :(

Yeah, let me take a look, if we can fix that. We have other repos working fine, so I think we need to add the go-setup step here as well.

We also need to merge this for the gomod to pass: metal3-io/project-infra#783

@tuminoid
Copy link
Member

@tuminoid looks like github actions only have 1.22.2 on their ubuntu runners :(

Yeah, let me take a look, if we can fix that. We have other repos working fine, so I think we need to add the go-setup step here as well.

We also need to merge this for the gomod to pass: metal3-io/project-infra#783

OK, so if we merge #43 and then rebase this, and also merge the project-infra PR for the gomod test, then I think this should pass.

@tuminoid
Copy link
Member

Also edit hack/gomod.sh to run Go 1.22 (fixes local runs).

/test gomod

@dtantsur dtantsur force-pushed the deps branch 2 times, most recently from 3c5525a to 92ad61b Compare June 10, 2024 11:22
hack/gomod.sh Outdated
"${CONTAINER_RUNTIME}" run --rm \
--env IS_CONTAINER=TRUE \
--volume "${PWD}:/workdir:ro,z" \
--entrypoint sh \
--workdir /workdir \
docker.io/golang:1.21 \
docker.io/golang:${GO_VERSION} \
Copy link
Member

Choose a reason for hiding this comment

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

This works, but it is diverging what project-infra/CI tests vs what you locally test.

CI uses golang:1.22, so that is suggested to be used here as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it's a bit annoying to hardcode different variations of the same thing everywhere. Should we maybe have a text file that will provide a version that the CI uses? Or somehow de-duplicate it?

Copy link
Member

Choose a reason for hiding this comment

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

Golang bump is the least annoying and least coupled of them all, since you need to bump it once per minor version of golang, ie. once every six months.

Project-infra vs repository coupling is annoying I agree, as I've done multiple non-compatible bumps to other linters etc and it requires a lot of juggling and coordination. So far we've deemed it to happen rarely enough to use time and effort to replace it as its how the Prow testing works right now with a dozen repos structure we have in place.

We're about to pick up active development here again, let's make sure
to work with the latest stuff to avoid accidentally depending on old and
incompatible versions (looking at you, controller-runtime).

Forces bumping the Go version to 1.22 (using 1.22.3/4 to avoid reverting
the fix for https://osv.dev/vulnerability/GO-2024-2687).

Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

AFAIK this looks like solid golang bump.

/lgtm

Maybe update title to reflect that as its more about golang bump than deps.

@metal3-io-bot
Copy link
Contributor

@tuminoid: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

AFAIK this looks like solid golang bump.

/lgtm

Maybe update title to reflect that as its more about golang bump than deps.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@dtantsur
Copy link
Member Author

It was about dependencies initially, Go is sort of forced on me :)

/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtantsur

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 10, 2024
@dtantsur dtantsur merged commit c183721 into metal3-io:main Jun 10, 2024
7 of 8 checks passed
@dtantsur dtantsur deleted the deps branch June 10, 2024 12:00
@dtantsur
Copy link
Member Author

@tuminoid: adding LGTM is restricted to approvers and reviewers in OWNERS files.

Indeed: #44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants