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.

Sign up for GitHub

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

Jump to bottom

Use a SourcedImage struct in c/image/copy #1578

Merged
merged 10 commits into from
Jun 15, 2022
Merged

Use a SourcedImage struct in c/image/copy #1578

merged 10 commits into from
Jun 15, 2022

Commits on Jun 14, 2022

  1. Have image.FromUnparsedImage return a SourcedImage instead of types.I… 

    …mage

This will allow us to add more methods to SourcedImage without breaking
external implementations of types.Image, if any.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
    @mtrmac
    mtrmac committed Jun 14, 2022
    Configuration menu
    Copy the full SHA
    0cf416b View commit details
    Browse the repository at this point in the history

  2. Make SourcedImage.{ManifestBlob,ManifestMIMEType} public 

    We know these values are available without having to worry about
errors, which allows simplifying users in c/image/copy.  We don't really
need accessor functions, all users are internal (and a public field
is actually better protected than a public method, because a public
method can be used by third parties through a caller-defined interface).

It's also usefo to make them available as individual values; various
users only need one or the other.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
    @mtrmac
    mtrmac committed Jun 14, 2022
    Configuration menu
    Copy the full SHA
    2874e99 View commit details
    Browse the repository at this point in the history

  3. Introduce manifestConversionPlan 

    To make determineManifestConversion easier to test,
we'll want to separate it from imageCopier and make it a pure function.

To do that, start with introducing a struct for its return value;
that way we won't need three unnamed return values, and three
variables with long names at the call site.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
    @mtrmac
    mtrmac committed Jun 14, 2022
    Configuration menu
    Copy the full SHA
    cba0127 View commit details
    Browse the repository at this point in the history

  4. Move the manifest conversion setup out of determineManifestConversion 

    Have determineManifestConversion just set a flag in the returned plan;
then copier.copyOneImage will actually act on that flag.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
    @mtrmac
    mtrmac committed Jun 14, 2022
    Configuration menu
    Copy the full SHA
    225269f View commit details
    Browse the repository at this point in the history

  5. Use manifestConversionPlan for expected results in TestDetermineManif… 

    …estConversion

This is not smaller, but a bit more readable and consistent
with the real call site.

Also, on test failures we can see the full returned plan as a single unit,
making it possibly easier to see what went wrong.

Should not change (test) behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
    @mtrmac
    mtrmac committed Jun 14, 2022
    Configuration menu
    Copy the full SHA
    e117048 View commit details
    Browse the repository at this point in the history

  6. Introduce determineManifestConversionInputs 

    ... and split the actual logic (determineManifestConversion)
from imageCopier (imageCopier.determineManifestConversion).

determineManifestConversion is now a pure function (well,
apart from logging).

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
    @mtrmac
    mtrmac committed Jun 14, 2022
    Configuration menu
    Copy the full SHA
    f395eeb View commit details
    Browse the repository at this point in the history

  7. Mostly test the pure function in TestDetermineManifestConversion 

    Keep the "reading manifest failed" case, for now.

Should not change (test) behavior, except that the test coverage
is now a tiny bit smaller - but then the test not depending
on imageCopier is the primary reason for this refactoring.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
    @mtrmac
    mtrmac committed Jun 14, 2022
    Configuration menu
    Copy the full SHA
    7602cbd View commit details
    Browse the repository at this point in the history

  8. Record an *image.SourcedImage in imageCopier.src 

    This is surprisingly somewhat convoluted.

We've had to do a bunch of work to make determineManifestConversion
not depend on a complete (and mostly valid!) image, to be able
to make this change.

Then, so that we don't lose test coverage (and to actually benefit
from the new private type), use SourcedImage.ManifestMIMEType
in imageCopier.determineManifestConversion.  That allows us to get
rid of the last imageCopier.determineManifestConversion call
in tests.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
    @mtrmac
    mtrmac committed Jun 14, 2022
    Configuration menu
    Copy the full SHA
    cce475b View commit details
    Browse the repository at this point in the history

  9. Use StoredImage.StoredManifest() where appropriate 

    This eliminates some unreachable error paths.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
    @mtrmac
    mtrmac committed Jun 14, 2022
    Configuration menu
    Copy the full SHA
    f074600 View commit details
    Browse the repository at this point in the history

  10. Inline imageCopier.determineManifestConversion into the caller 

    It's now basically a trivial call forwarder we don't need for
anything, so just have the caller set determineManifestConversionInputs
directly (and let it benefit from labeling the inputs).

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
    @mtrmac
    mtrmac committed Jun 14, 2022
    Configuration menu
    Copy the full SHA
    acbde71 View commit details
    Browse the repository at this point in the history