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.

Sign up for GitHub

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

Jump to bottom

vendor to latest c/{common,image,storage} #5929

Merged
merged 2 commits into from
Jan 24, 2025
Merged

vendor to latest c/{common,image,storage} #5929

merged 2 commits into from
Jan 24, 2025

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Jan 21, 2025

Make sure all the test pass before we do a final vendor dance.

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

@nalind
Copy link
Member

nalind commented Jan 21, 2025

Looks like we need to update

buildah/tests/run.bats

Line 974 in eac9331

for mask in /proc/acpi /proc/kcore /proc/keys /proc/latency_stats /proc/sched_debug /proc/scsi /proc/timer_list /proc/timer_stats /sys/dev/block /sys/devices/virtual/powercap /sys/firmware /sys/fs/selinux; do
to account for containers/common#2278.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 22, 2025
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 22, 2025
@Luap99
Copy link
Member Author

Luap99 commented Jan 22, 2025

@mtrmac Tests are broken, it seems to be from your new code containers/image#2613?!

[+1757s] # Error: writing blob: layer 0 (blob "sha256:85a67008b2828ae9236a430a302a14053eaec77addb721865cc4857147a94b2f"/""/"sha256:85a67008b2828ae9236a430a302a14053eaec77addb721865cc4857147a94b2f") does not match config's DiffID "sha256:d2421964bad195c959ba147ad21626ccddc73a4f2638664ad1c07bd9df48a675"

@mtrmac
Copy link
Contributor

mtrmac commented Jan 22, 2025

@mtrmac Tests are broken, it seems to be from your new code containers/image#2613?!

That very likely needs an equivalent of containers/podman@5235ee5 ; I’ll take a look and file a PR today.

@mtrmac mtrmac mentioned this pull request Jan 22, 2025
Merged
mtrmac
mtrmac reviewed Jan 22, 2025
View reviewed changes
go.mod Outdated
Comment on lines 10 to 11
github.com/containers/common v0.61.1-0.20250121185748-34a90afcdc6d
github.com/containers/image/v5 v5.33.1-0.20250121231649-a45ebe065b9e
Copy link
Contributor

Choose a reason for hiding this comment

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

These are, nominally, downgrades.

For c/image, containers/image#2691 should help.

Copy link
Member Author

Choose a reason for hiding this comment

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

They are not downgrades?!
34a90afcdc6d (containers/common@34a90af)
a45ebe065b9e (containers/image@a45ebe0)

These are (or were) commits from the latest main branch at the time I did the vendor. So they are not downgrades if we consider main always to be newest which it must be.

Copy link
Contributor

Choose a reason for hiding this comment

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

You’re right that main is newer — it’s just that Go doesn’t know that.

@mtrmac
Copy link
Contributor

mtrmac commented Jan 22, 2025

I’ll take a look and file a PR today.

#5932 .

@Luap99 Luap99 mentioned this pull request Jan 23, 2025
Open
@TomSweeneyRedHat
Copy link
Member

Changes LGTM, looks like you may need to rebase.

Luap99 added 2 commits January 24, 2025 11:45
@Luap99
vendor to latest c/{common,image,storage}
4887a4f 
Make sure all the test pass before we do a final vendor dance.

Signed-off-by: Paul Holzinger <[email protected]>
@Luap99
tests: remove masked /sys/dev/block check
2119c8d 
The c/common defaults were changed to no longer mask this path[1]. As
such we need to remove it from this test.

[1] containers/common#2278

Signed-off-by: Paul Holzinger <[email protected]>
@Luap99
Copy link
Member Author

Luap99 commented Jan 24, 2025

@TomSweeneyRedHat @nalind This should be good to merge. Then we can vendor buildah@main on the podman side

@nalind
Copy link
Member

nalind commented Jan 24, 2025

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 24, 2025
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 24, 2025

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@openshift-merge-bot openshift-merge-bot bot merged commit 21fe6dc into containers:main Jan 24, 2025
32 checks passed
@Luap99 Luap99 deleted the vendor branch January 24, 2025 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtrmac mtrmac mtrmac left review comments

Assignees

@nalind nalind

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Luap99 @nalind @mtrmac @TomSweeneyRedHat @openshift-merge-robot