Add option to preserve digests #1413
Conversation
There was a problem hiding this comment.
Thanks! This is great to have.
One thing I’m wondering about are the error messages … ”we are preserving digests” is not going to be necessarily obvious to users a few levels up a call stack, e.g. if the image is signed. Maybe that should be something like “we can’t change the digest (image is singled)”, with the part in parenthesis determined when making the canModify… = false decision and stored in copier/imageCopier. (Or do others think this is good enough and we don’t need that over-engineering?)
Jamstah
force-pushed
the
preserve-digests
branch
from
02c5e17
to
adc2d41
Compare
I considered this, but thought it would be a bit messy. You're right though, it will be more usable. I've had a go at an impl, see what you think. |
Jamstah
force-pushed
the
preserve-digests
branch
3 times, most recently
from
8f3f396
to
6dbe0ff
Compare
Jamstah
changed the title
There was a problem hiding this comment.
LGTM apart from the trivial nits. Thanks!
@vrothberg I’d appreciate a second opinion.
Jamstah
force-pushed
the
preserve-digests
branch
from
bd85387
to
2114d46
Compare
A digest-stable copy seems popular, even when not copying signed images. Using --all can still change digests. Adding an option to ensure digests are preserved. Also adding a missing check to enable digest preservation for manifest lists where the destination is digested. See: containers/skopeo#1440 containers/skopeo#1378 containers/skopeo#1102 containers/skopeo#1451 Signed-off-by: James Hewitt <james.hewitt@uk.ibm.com>
Jamstah
force-pushed
the
preserve-digests
branch
from
2114d46
to
e1ee980
Compare
| // Determine if we're allowed to modify the manifest list. | ||
| // If we can, set to the empty string. If we can't, set to the reason why. | ||
| cannotModifyManifestListReason := "" | ||
| if len(sigs) > 0 { |
There was a problem hiding this comment.
I'd prefer this to be a switch { case: } or is it intended to possibly override the reason?
There was a problem hiding this comment.
Any of the three conditions, possibly all three, can be true simultaneously.
I guess we could build a list here, but I’m personally not really worried about that, and not at all sure it’s worth the complexity.
| } | ||
| if options.PreserveDigests { | ||
| cannotModifyManifestReason = "Instructed to preserve digests" | ||
| } |
There was a problem hiding this comment.
This looks like a code clone above. It could be worth breaking that into a separate function to prevent diverging in the future.
There was a problem hiding this comment.
(It seems likely, but not quite obvious, to me, that the two are going to stay in sync.)
@vrothberg did you have in mind a function with three boolean parameters? A struct with named fields, to avoid three boolean values in random order? Or something with a copier, options, sigs parameters?
At 2 iterations, just comments linking the two (“See also $function”) might be a low-tech approach.
There was a problem hiding this comment.
At 2 iterations, just comments linking the two (“See also $function”) might be a low-tech approach.
That sounds good to me. I just want to make sure we don't miss that in the future. A breadcrumb as you suggested should do the trick.
There was a problem hiding this comment.
It does feel that there is a cleaner version hidden, waiting to be discovered (e.g. the “get signatures that we want to copy” code can almost certainly be consolidated between the two paths), but at least the destIsDigestedReference check is legitimately different — so many intermediate “correct” versions are going to be more complex, and refactoring this to be clean and simple feels rather out of scope of this contributed PR.
There was a problem hiding this comment.
Agreed, let's merge and keep it in the back of our heads.
There was a problem hiding this comment.
Just to tug on this thread, #1417 is a tentative idea. I don’t think that one is actually worth it as is.
There was a problem hiding this comment.
… and #1418 for the comment pair.
Add option to preserve digests
A digest-stable copy seems popular, even when not copying signed images.
Using --all can still change digests. Adding an option to ensure digests
are preserved.
Will follow up with a skopeo PR if/when this is merged to use it.
See:
containers/skopeo#1440
containers/skopeo#1378
containers/skopeo#1102
containers/skopeo#1451
Signed-off-by: James Hewitt james.hewitt@uk.ibm.com