#305 cleanup 2: Reference handling #426
Conversation
|
More questions:
|
|
The |
mtrmac
force-pushed
the
305-cleanup-2
branch
2 times, most recently
from
1d06cf1 to
32d7287
Compare
mtrmac
force-pushed
the
305-cleanup-2
branch
2 times, most recently
from
8d89188 to
c832106
Compare
| type storageImageDestination struct { | ||
| imageRef storageReference // The reference we'll use to name the image | ||
| publicRef storageReference // The reference we return when asked about the name we'll give to the image | ||
| imageRef storageReference |
There was a problem hiding this comment.
nit: Do we need a comment here?
There was a problem hiding this comment.
“A reference to the image this destination writes to” should be clear enough without a comment; the previous two values needed comments (even more comments, arguably) to explain when to use which value.
| func imageMatchesRepo(image *storage.Image, ref reference.Named) bool { | ||
| repo := ref.Name() | ||
| for _, name := range image.Names { | ||
| if named, err := reference.ParseNormalizedNamed(name); err == nil { |
There was a problem hiding this comment.
Is it ok to ignore the err? Should we logrus.Debugf this or is this expected.
There was a problem hiding this comment.
imageMatchesRepo is clearly(?) false for values which can’t be parsed. I don’t know whether such values can happen, or how likely that is. @nalind ?
(This code is not new, it’s only moved out of the calling function (commit “UNTESTED: Extract duplicate code from resolveImage”).)
|
@umohnani8 @runcom @giuseppe PTAL |
| // and would prefer to receive an unmodified manifest instead of one modified for the destination. | ||
| // Does not make a difference if Reference().DockerReference() is nil. | ||
| func (d *ostreeImageDestination) IgnoresEmbeddedDockerReference() bool { | ||
| return false // N/A, DockerReference() returns nil. |
There was a problem hiding this comment.
when it is not used, should it return true so that updateEmbeddedDockerReference exits early?
There was a problem hiding this comment.
@giuseppe I’m not too worried about saving a few cycles during an image copy which writes gigabytes of data to the disk.
If anything, it’s more a matter of semantics: does the ostree backend prefer a manifest tailored for it, or an unmodified one? Looking at how ostreeImageDestination.Commit records the manifest digest in the ostree metadata, maybe it really would prefer the unmodified one, and this should return true for that reason.
If it does not matter at all, I’d rather leave it at the default false; we may eventually want to to have defaults for many of the ImageDestination flags without copies in every transport, and that will result in shorter code if more transports use the default values.
mtrmac
force-pushed
the
305-cleanup-2
branch
2 times, most recently
from
f92315e to
1a2c7a9
Compare
mtrmac
force-pushed
the
305-cleanup-2
branch
3 times, most recently
from
b815d5b to
87b4920
Compare
|
@mtrmac Needs rebase, What do we need to do to get this moving forward? |
|
At least one LGTM would be nice. I can technically force-merge it without one. |
|
LGTM |
|
do we want to drop the RFC and UNTESTED tags from the patches before merging? |
... using validReferenceTestCases. The cases in that table are a superset of the two previously used cases. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This does not change the implementation at all yet. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…aces ... if PolicyConfigurationIdentity includes @id. At least the :tag form is clearly useful. This does not handle the name:tag@digest case (which _can_ happen), where the tag is currently recorded inside s.name but not s.tag; the possible code handling it would be very non-obviously pointing out this difference. For now, only leave a FIXME in the test. Maybe it should be handled by refusing such input instead; e.g. that's what docker.Transport does. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
For some unfanthomable reason I wrote the test to accept non-canonical inputs to ParseReference, instead of the canonicalized prefixes returned by PolicyConfigurationIdentity and PolicyConfigurationNamespace. So, replace the expected cases by the actually returned forms, up to the full name:tag@digest@ID, and the relevant prefixes. This only changes the tests; the cases which would fail are commented out for now. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Re-enable the failing tests, and rewrite the implementation to match expected inputs. Notably this re-enables the @id form with no name. Like docker.Transport, this does no validation of the name part; using the docker/reference parses is not correct because they are not intended to accept e.g. hostname-only "docker.io" Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Before touching the internals of reference representation, make sure the expected values of verboseName are known so that we don't break any users by touching the internals. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
All existing users pass a reference.Named, and check for nil before calling this function. This only changes the interface; the implementation will be simplified presently. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Avoid a few redundant completeReference != nil checks Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Now that the implementation transition is complete, the value being "complete" is no longer a differentiating factor; instead, use a shorter, easier to use name. (We could also use just "ref", but there are several kinds of references around, this suggests more about the type.) Signed-off-by: Miloslav Trmač <mitr@redhat.com>
The code assumes that anyway, and this allows us to move an unreachable check from ParseStoreReference into a testable one in newReference. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Instead of three cases, each handling a subset of fields, construct the return value in steps, making it clear how the fields fit together. (The code could, in theory, return just a [storeSpec] with no name nor ID; but newReference has just been edited to refuse to create such values.) Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Instead of first extracting fields, and only later deciding based on their existence and contents how to interpret them, combine the field extraction and interpretation into one step. We may possibly check the trailing field twice if it is a digest, but overall it's simpler. NOTE: Perhaps I am overlooking a reason the code was organized this way; specific error reporting behavior perhaps? Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Digests are only valid for named references, so the two are mutually exclusive and order does not quite matter. To avoid a costly store.Image() lookup for named references, check for "@:" first. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Instead, just let ParseNormalizedNamed do it. This saves us from validating the digest, from checking for name@digest@digest, and from integrating the digest back into "named". Much simpler overall. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Again, as suggested elsewhere, maybe we should just refuse such images. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
If we find an image and determine the ID, it seems pointless to load it again by ID immediately afterwards. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This allows a destination to opt out of updating the embedded name:tag in schema1 manifests without violating the ImageDestination.Reference / ImageReference API expectations. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…n.publicRef Now that storageImageDestination.IgnoreEmbeddedDockerReference exists, this hack is no longer needed. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
“RFC: Refuse un-named @digest reference input” is pretty fundamental to this PR, the final form of “RFC: Simplify parsing of image IDs/digests” is not essential, but it is a prerequisite for “Don’t explicitly parse a digest.”; together they are a fairly notable simplification. (The functional aspects of them have pretty good test coverage; I just don’t know whether there was any other deliberate intent in structuring the code that way, maybe for error reporting reasons.) “RFC UNTESTED: Only load an image once in resolveImage” can be dropped without impact on the rest. It’s been 3 months without any response to the RFCs, so I am not too keen on continuing to wait for them. “UNTESTED: Fix name@digest@IDprefix parsing” could, I suppose, be dropped, by re-creating the presumed bug in other versions of the code — but the result would be just as untested in that case; or we could drop the “RFC: Simplify parsing of image IDs/digests”+“Don’t explicitly parse a digest.” commits as well and give up on simplifying I can easily enough drop “UNTESTED: Match repositories using .Name() instead of FamiliarName()” and “UNTESTED: Extract duplicate code from resolveImage”, though I am fairly confident about them—they are trivial refactorings. As an alternative, would it be easy enough to run the libpod / buildah test suites against this PR’s branch of containers/image? Is there a test harness that can do that, or can I do that in a VM easily? |
This is purely to test whether containers/image#426 breaks anything. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is purely to test whether containers/image#426 breaks anything. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is purely to test whether containers/image#426 breaks anything. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is purely to test whether containers/image#426 breaks anything. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is purely to test whether containers/image#426 breaks anything. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is purely to test whether containers/image#426 breaks anything. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is purely to test whether containers/image#426 breaks anything. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is purely to test whether containers/image#426 breaks anything. NOTE: This is not even a correct vendoring, it drops a c/i/docker/docker update which would require rebasing github.com/docker/docker. ALL this does is update c/i/storage, and c/i/copy. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
I was being stupid, of course I can just take advantage of the GitHub CI integration:
All of them are green, so I’ll merge this as is. |
|
👍 |
3 participants
This is a follow-up, and on top of, #416. Unlike that one, which should be overwhelmingly uncontroversial, this one involves a few design choices.
See individual commit messages for more details. (…and not to read #416 again; start at “Make TestStorageReferenceDockerDeference table-driven“.); please pay particular attention to the “RFC” and “UNTESTED” marked ones. I’ll be happy to break this up into several PRs if any part needs more discussion than others.
General themes:
@digest without a name, or name@digest@digest@ID:tag@digest, which is actually a recognized syntax. (Or would we refuse it instead?)verboseNamestorageReferenceto be a (store,reference.Named, ID) triplet, without three redundant string members, over about 20 tiny commits.ParseStoreReference(unless I’m missing a reason for the code to be the way it is)types.ImageDestination.EmbeddedDockerReference, instead of inconsistently lying instorageImageDestination.Reference.DockerReference().@nalind @runcom PTAL.