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

Refactor blob copying and compression #1579

Merged
merged 35 commits into from Jun 16, 2022
Merged

Refactor blob copying and compression #1579

merged 35 commits into from Jun 16, 2022

Commits on Jun 15, 2022

  1. Move imageCopier.copyBlobFromStream into a new copy/blob.go 

    The over-300-line-long function would benefit from splitting up,
and copy/copy.go has grown entirely too long.

As a first step, move it to a new copy/blob.go.

Only moves unmodified code, should not change behavior.

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

  2. Move errorAnnotationReader 

    ... so that the file reads from top-level
callers down to implementation details.

Only moves unchanged code, should not change behavior.

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

  3. Rename the srcStream parameter to srcReader 

    We will use srcStream for another purpose.

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

  4. Introduce sourceStream, use it for the first pipeline stage 

    The purpose of sourceStream is to make it a bit easier
to split the blob copy pipeline stages into separate functions;
they will update a *sourceStream, which we can pass a single parameter.

For now, build it and use it to initialize a digestingReader; then
update other code that should consume the current pipeline data.

NOTE: That is a judgement call, it is not always obvious what data
was intended to be used and what is a bug.

As a geneeral principle, use srcInfo (the original unmodified data)
in user-visible error reports, notably using the original digest.
Otherwise, lean towards using srcStream.info.

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

  5. Use stream.reader instead of destStream 

    One part of moving to use stream throughout the pipeline.

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

  6. Eliminate inputInfo 

    Use stream.info throughout.

Should not change behavior.  (In some cases that's
dubious, we are currently losing data like annotations
compression/decompression.  This commit doesn't
change that, only adds a FIXME? to eventually review.)

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

  7. Move the OCI blob decryption pipeline step into copier.blobPipelineDe… 

    …cryptionStep

Only moves the code with minimal necessary adjustments, should not change behavior.

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

  8. Beautify blobPipelineDecryptionStep a bit 

    Should not change behavior.

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

  9. Rename newDesc to desc 

    This smaller function can use simpler identifiers.

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

  10. Beautify the DecryptLayer usage a bit 

    Should not change behavior.

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

  11. Move the OCI blob encryption pipeline step into copier.blobPipelineEn… 

    …cryptionStep

Only moves the code with minimal necessary adjustments, should not change behavior.

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

  12. Simplify blobPipelineEncryptionStep 

    Make the "not encrypting" case truly special, and then we
don't need extra variables across the function.

Should not change behavior.

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

  13. Exit from updateCryptoOperationAndAnnotations early if not encrypting 

    Should not change behavior.

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

  14. Beautify blobPipelineEncryptionStep 

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

  15. Rename copy/encrypt.go to copy/encryption.go 

    It contains decompression code as well.

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

  16. Move the compression detection step into blobPipelineDetectCompressio… 

    …nStep

Only moves the code with minimal necessary adjustments, should not change behavior.

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

  17. Beautify blobPipelineDetectCompressionStep 

    We can use shorter variable names in this short function now.

Should not change behavior.

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

  18. Move the compression annotation update closer to the other compressio… 

    …n edits

Only moves code, should not change behavior.

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

  19. Move copier.compressGoroutine to compression.go 

    Only moves unchanged code, should not change behavior.

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

  20. Move the compression/decompression step to blobPipelineCompressionStep 

    Only moves the code with minimal necessary adjustments, should not change behavior.

Those adjustments are not completely trivial in this case: we could just do a
"defer stream.Close()" in copyBlobFromStream, now we need a separate closers field in
blobPipelineCompressionStepData.

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

  21. Rename detectedCompession to detected 

    The compression meaning is clear in context here.

Should not change behavior.

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

  22. Rename compresisonOperation to operation 

    Should not change behavior.

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

  23. Rename uploadCompressionFormat to uploadedAlgorithm 

    Should not change behavior.

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

  24. Rename uploadCompressorName to uploadedCompressorName 

    Should not change behavior.

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

  25. Rename compressionMetadata to uploadedAnnotations. 

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

  26. Compute srcCompressorName already in blobPipelineDetectCompressionStep 

    Except for the encrypted content case, we already have the data available,
and this will allow us to just pass around a *blobPipelineDetectCompressionStepData
without an extra parameter for srcCompressorName.

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

  27. Return a fresh &bpCompressionStepData in each case 

    That's actually shorter because in most cases we just
use a field initializer instead of setting a variable;
and we don't need that variable in the first place.

For now, do just the minimal edit, we will clean this up
a bit later.

Should not change behavior.

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

  28. Move uploadedAnnotations inside the individual cases. 

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

  29. Move uploadedAlgorithm inside the individual cases. 

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

  30. Clean up the control flow of blobPipelineCompressionStep 

    Don't use else after each step that returns early.

The function nows looks like a set of ~special cases,
with the general fallback of doing nothing.

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

  31. Rename s to decompressed in the re-compress case 

    to avoid ambiguity.

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

  32. Factor out copier.compressedStream from copier.blobPipelineCompressio… 

    …nStep

... so that we only implement the pipe handling once.

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

  33. Split blobPipelineCompressionStep into individual functions 

    The logic is a simple sequence of condition + decision,
which is fairly regullar, and doesn't have to be in a single function.

So, split it up.

Eliminate the recently-introduced closers array again, in most cases.

Should not change behavior.

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

  34. Eliminate the closers array in bpcRecompressCompressed 

    ... now that it only could effectively apply to a single element.

Shoud not change behavior.

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

  35. Use a loop for the alternatives in blobPipelineCompressionStep 

    The alternatives are, mostly, things to _try_; we can typically
fall back to just using the original.

So, just use a loop to take advantage of the regular structure.

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