#305 cleanup #416
Merged
#305 cleanup #416
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This breaks API, so tests are going to fail; corresponding skopeo PR is containers/skopeo#477 . |
mtrmac
force-pushed
the
305-cleanup
branch
2 times, most recently
from
d883d69 to
c4cdaa4
Compare
Now that inspectManifest only forwards the call to
genericManifest.imageInspectInfo, simplify the call chain from
{memoryImage,sourcedImage}.Inspect→inspectManifest→genericManifest.imageInspectInfo
to genericManifest.Inspect directly (via embedding genericManifest into
{memoryImage,sourcedImage}).
Signed-off-by: Miloslav Trmač <[email protected]>
... instead of inventing a nonsensical value if the field is missing in OCI images. Signed-off-by: Miloslav Trmač <[email protected]>
The whole purpose of UnparsedImage is to not parse the manifest, so having this interface which more or less requires the manifest to be parsed and recomputed is not desirable. sourcedImage now uses ImageSource.LayerInfosForCopy via the private UnparsedImage.src, which is a bit ugly, but that dependency has already existed, and we get a more consistent public API for this price. Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
Drop unused private fields in types used only for JSON marshaling. Use Schema2V1Image for the pre-1.8.3 reencoding, to mirror what docker/docker does. It is especially pointless to explicitly declare extra fields to preserve when the immediately following code removes all of them. Signed-off-by: Miloslav Trmač <[email protected]>
... which is what the API contract says it should do, at least. Signed-off-by: Miloslav Trmač <[email protected]>
…onversion Signed-off-by: Miloslav Trmač <[email protected]>
... to avoid confusion with image.manifestSchema1.convertToManifestSchema2 Signed-off-by: Miloslav Trmač <[email protected]>
... instead of reloading it in LayerInfosForCopy and getSize. Alternatively, it seems we could just save the TopLayer field; this is more generic. AFAICT this should be equally safe in principle: any locking done in the storage.Store loaders is unlocked by the time we get the *storage.Image, so if anything can modify the unlocked *storage.Image concurrently after this change, or make its fields invalid, the same breakage can in principle happen before this change just as well, "only" with a shorter window. Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
Image.UpdatedImage is supposed to create a new instance which does not modify the original, so Manifest.UpdateLayerInfos should not modify the original either. Signed-off-by: Miloslav Trmač <[email protected]>
Not _quite_ comprehensive (notably not covering most failure cases), but at least comparable with the schema2 and OCI tests. Signed-off-by: Miloslav Trmač <[email protected]>
|
@runcom ping |
|
LGTM, @nalind please ack and I'll merge |
nalind
reviewed
Mar 13, 2018
| } | ||
| if s1.Config != nil { | ||
| i.Labels = s1.Config.Labels | ||
| } |
There was a problem hiding this comment.
Does this leave i.Labels set to nil if the compat blob doesn't specify any labels?
There was a problem hiding this comment.
“A nil map is equivalent to an empty map except that no elements may be added.”, which seems acceptable here (and consistent with schema2).
|
One nit, otherwise LGTM. |
|
👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a set of quite independent cleanups and minor bug fixes on top of #305 . Notably resolves #413, and adds tests for the schema1 code to decrease risk of similar breakage in the future.
See individual commit messages for more details. I’ll be happy to break this up into several PRs if any part needs more discussion than others.
Cc: @nalind
types.Image.Inspectcall chainImageInspectInfo.CreatedoptionalLayerInfosForCopyfromUnparsedImagetoImage/sourcedImagemanifest.Schema1.Inspect""incomputeIDfor unknown manifest typesmanifest.Schema1.ToSchema2toToSchema2Config*storage.Imageonly once instorageImageSourcestorageImageDestination.{image,systemContext}Schema1.UpdateLayerInfosimage.manifestSchema1