-
Notifications
You must be signed in to change notification settings - Fork 369
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
Fix conversion determination when encrypting #1930
Conversation
- When encrypting, actually make a choice even if the destination does not care about manifest formats - Don't try converting to formats that don't support encryption at all - Fail with a readable error messsage instead of "internal error" if we filter out all candidates. Signed-off-by: Miloslav Trmač <[email protected]>
…geCopier We will want to access imageCopier.cannotModifyManifestReason. All the ...Step functions are now methods of imageCopier, which is more consistent. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Exit early if we are not changing anything, to make the "no change" case easier to follow, and most of the code less indented. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
We would fail with an internal error anyway, this fails explicitly. Signed-off-by: Miloslav Trmač <[email protected]>
}, nil | ||
} | ||
|
||
if ic.cannotModifyManifestReason != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not immediately sure how this interacts with podman pull
of digest references, I need to test that. (It seems to me that that never worked, in which case this is, I guess, good enough…)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, no need to test, this is tracked as #1768 . I’ll just add a comment here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… not even a comment; #1768 would probably be cleanest to fix by recording the original manifest, and the updated manifest, separately; and in that case cannotModifyManifestReason
would not be set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
See individual commit messages for a bit more detail.