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

[release-5.19 backport] Bump github.com/containers/ocicrypt to 566b808 #1539

Merged
merged 1 commit into from May 9, 2022

Conversation

lsm5
Copy link
Member

@lsm5 lsm5 commented May 4, 2022

The two latest commits in c/ocicrypt get rid of OAEPDefaultHash envvar
and default to sha256.

They are waiting for confirmation on their rust side before cutting a
release. So, hoping to get this in assuming dependabot could take a
while.

Signed-off-by: Lokesh Mandvekar lsm5@fedoraproject.org

@vrothberg @mtrmac @rhatdan PTAL. This is for subsequent vendoring into podman v4.0-rhel branch.

RE: containers/podman#14102 (comment)

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

LGTM.

@mtrmac
Copy link
Collaborator

mtrmac commented May 4, 2022

The failing tests probably need some variant of #1535 ; I’m not 100% certain that the backport branch doesn’t need to deviate (Cc: @cevich ), but my first guess is that backporting that change as is is good enough, and probably worth doing even while waiting for a confirmation.

@lsm5
Copy link
Member Author

lsm5 commented May 4, 2022

Depends on #1540

@cevich
Copy link
Member

cevich commented May 4, 2022

backporting that change as is is good enough, and probably worth doing

Ya, I generally avoid changing out VM images on release branches, and this specific repo. has extra complicating factors. However there are sometimes exceptions. If this branch is reasonably new and sufficiently related to the skopeo-repo-state where F36 was introduced, then yes. Update the VM images here so they match the skopeo branch referenced for testing (which is also currently/probably set wrong).

@mtrmac
Copy link
Collaborator

mtrmac commented May 4, 2022

this branch is reasonably new and sufficiently related to the skopeo-repo-state where F36 was introduced, then yes.

It’s about a month of difference so far, and the original image is gone, so it seems easier to update to the current one than to branch and maintain another set of images, at least right now. Assuming we can actually get that working, the number of trivial changes being discovered one at a time is inching towards looking concerning.

@cevich
Copy link
Member

cevich commented May 4, 2022

It’s about a month of difference so far

Oh okay, that's not super bad, maybe it can be done.

the number of trivial changes being discovered one at a time is inching towards looking concerning.

Yes, there are differences in the images too (i.e. tooling script differences). Another reason generally I say they only move forward. I do have a super-squirrel-secret way to recover images within a 29-59-day window, but if the images age past 60-days, they're irrecoverable. I only mention that in case it's an option here, I haven't checked any CI-run dates on this branch.

@lsm5 lsm5 force-pushed the release-5.19-ocicrypt-bump branch from 3d4c5b3 to fe6fe7d Compare May 9, 2022 15:38
@lsm5
Copy link
Member Author

lsm5 commented May 9, 2022

@mtrmac this is good to go. PTAL

go.mod Outdated
@@ -42,3 +42,5 @@ require (
golang.org/x/sys v0.0.0-20220114195835-da31bd327af9
golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1
)

replace github.com/containers/ocicrypt => github.com/containers/ocicrypt v1.1.4-0.20220428134531-566b808bdf6f
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this line really necessary? I think just bumping the version above should be enough. (And it kind of has to be enough because replace directives only work at the top level, i.e. this wouldn’t apply to a Skopeo/Podman build.)

Copy link
Member Author

Choose a reason for hiding this comment

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

go mod download didn't let me fetch v1.1.4 so I kept the line with the version-commit as-is and got rid of the replace directive. PTAL.

Copy link
Collaborator

Choose a reason for hiding this comment

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

go mod download isn’t supposed to update go.mod I think…

FWIW go get github.com/containers/ocicrypt@v1.1.4 worked for me, resulting in

github.com/containers/ocicrypt v1.1.4

. I didn’t realize this pre-release version actually points at a tagged version — in that case, I think using the 1.1.4 tag would be a bit safer WRT any other consumers of that.

@lsm5 lsm5 force-pushed the release-5.19-ocicrypt-bump branch from fe6fe7d to 40f5a81 Compare May 9, 2022 17:14
The two latest commits in c/ocicrypt get rid of OAEPDefaultHash envvar
and default to sha256.

They are waiting for confirmation on their rust side before cutting a
release. So, hoping to get this in assuming dependabot could take a
while.

Signed-off-by: Lokesh Mandvekar <lsm5@fedoraproject.org>
@lsm5 lsm5 force-pushed the release-5.19-ocicrypt-bump branch from 40f5a81 to a9f5601 Compare May 9, 2022 18:19
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks for all the work to make this possible!

@lsm5
Copy link
Member Author

lsm5 commented May 9, 2022

Thanks @mtrmac .

Btw, on rawhide, I got this:

$ go get github.com/containers/ocicrypt@v1.1.4
go: github.com/containers/ocicrypt@v1.1.4: invalid version: resolves to version v1.1.5-0.20220428134531-566b808bdf6f (v1.1.4 is not a tag)

But then I simply edited go.mod to have v1.1.4 against ocicrypt, followed by only running go mod tidy (no go get), and seems to work well.

Maybe that's a new rawhide change.

@mtrmac
Copy link
Collaborator

mtrmac commented May 9, 2022

Or it could be something with GOPROXY, or a pre-existing local download? I find the tool’s behavior a bit hard to understand sometimes.

@mtrmac mtrmac merged commit 50dba28 into containers:release-5.19 May 9, 2022
@lsm5
Copy link
Member Author

lsm5 commented May 9, 2022

Or it could be something with GOPROXY, or a pre-existing local download? I find the tool’s behavior a bit hard to understand sometimes.

ack, I'll look further. Thanks a lot for the merge!!

@lsm5 lsm5 deleted the release-5.19-ocicrypt-bump branch May 9, 2022 18:38
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.

None yet

3 participants