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

vendor: update containers/{image,storage} #24054

Merged

Conversation

giuseppe
Copy link
Member

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 24, 2024
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@lsm5 lsm5 added the bloat_approved Approve a PR in which binary file size grows by over 50k label Sep 24, 2024
@giuseppe giuseppe force-pushed the vendor-image-storage-24-9-2024 branch from cb08fc6 to 7c3e2d1 Compare September 24, 2024 12:19
@lsm5
Copy link
Member

lsm5 commented Sep 24, 2024

centos-stream 9/10 don't have golang 1.22.6 so this is gonna fail on those envs. Do we need to ship this soon there? If not, then we can ignore centos failures.

@lsm5
Copy link
Member

lsm5 commented Sep 24, 2024

@giuseppe validate test is still failing

@giuseppe giuseppe force-pushed the vendor-image-storage-24-9-2024 branch from 7c3e2d1 to 05b35ed Compare September 24, 2024 12:39
go.mod Outdated
go 1.22.0
go 1.22.6

toolchain go1.22.7
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong, we do not want to force newer toolchains for no reason see the comment above.

Copy link
Member Author

Choose a reason for hiding this comment

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

I reverted this part, pushed a new version let's see if it passes the validation...

Copy link
Member

Choose a reason for hiding this comment

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

You cannot revert the go parts as this is hard enforced by go module now to be the minimum version used for builds. So if any dependency is using this we can either not bump the dep or have to live with that.

What we do control is the toolchain line and for this one we should never use it as the only purpose would be to force a newer version (compared to the go line) to compile with.

Copy link
Member Author

Choose a reason for hiding this comment

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

unfortunately that doesn't solve the problem with centos-stream 9/10.

The updated go part is coming from containers/image. I either drop the c/image bump patch or we accept these failures for now.

Copy link
Member

Choose a reason for hiding this comment

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

Well I brought that up when we did the update to go 1.22, I consider bumping to a z version as minimum fundamentally broken behavior that simply should not happen in a stable language but go decided to allow this so now we are forced to live with that if we want to consume dependencies that just force "random" go updates and yes this means we will be unable to build in slower updating envs. I don't know how much of a problem this will be in practise but I guess if we cannot build on RHEL or even fedora we have a rather big problem.

ref containers/image#2550 (comment)
and we decided in standup that we are ok to bump z versions.

@giuseppe giuseppe force-pushed the vendor-image-storage-24-9-2024 branch 2 times, most recently from 6a41271 to dba61ff Compare September 24, 2024 13:22
@giuseppe giuseppe added the No New Tests Allow PR to proceed without adding regression tests label Sep 24, 2024
@giuseppe giuseppe force-pushed the vendor-image-storage-24-9-2024 branch from dba61ff to 71af6ea Compare September 24, 2024 13:37
@Luap99
Copy link
Member

Luap99 commented Sep 24, 2024

Tests are failing

@giuseppe giuseppe force-pushed the vendor-image-storage-24-9-2024 branch 2 times, most recently from c7922f7 to c17c8f7 Compare September 24, 2024 15:31
Copy link

Cockpit tests failed for commit c7922f7d0e16189799ab7ef0926832d80a3c0301. @martinpitt, @jelly, @mvollmer please check.

@giuseppe giuseppe marked this pull request as draft September 24, 2024 16:38
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2024
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@giuseppe giuseppe force-pushed the vendor-image-storage-24-9-2024 branch from 894be15 to 7f29233 Compare September 24, 2024 18:32
@giuseppe
Copy link
Member Author

Tests are failing

fixed now

@giuseppe giuseppe marked this pull request as ready for review September 24, 2024 19:18
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2024
@rhatdan
Copy link
Member

rhatdan commented Sep 24, 2024

LGTM

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

openshift-ci bot commented Sep 25, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, Luap99

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

@Luap99
Copy link
Member

Luap99 commented Sep 25, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 25, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 1ae4b04 into containers:main Sep 25, 2024
82 of 88 checks passed
Luap99 added a commit to Luap99/libpod that referenced this pull request Sep 26, 2024
The go version there is only go 1.22.5 but we need go 1.22.6 as of
containers#24054

It is not clear to me how to best monitor the repos there to see when
they get the update. And then there is the fear that podman keeps
updating faster then these envs which makes testing there immposible[1]

[1] containers/image#2550 (comment)

Signed-off-by: Paul Holzinger <[email protected]>
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. bloat_approved Approve a PR in which binary file size grows by over 50k lgtm Indicates that a PR is ready to be merged. No New Tests Allow PR to proceed without adding regression tests release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants