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.

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

copy/multiple: split selection of images to be copied in copyMultipleImages #1982

Merged
merged 1 commit into from
Jun 6, 2023

Conversation

flouthoc
Copy link
Contributor

@flouthoc flouthoc commented Jun 4, 2023

Following PR splits copyMultipleImages into a new function called
prepareCopyInstances which returns a list of instanceCopy.

prepareCopyInstances does that in these two steps

  • How to copy via instanceCopyKind
  • What to copy by the content of the returned list.

See newly added unit test to see how it is used.

@flouthoc
Copy link
Contributor Author

flouthoc commented Jun 4, 2023

@mtrmac PTAL

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well enough right now, but it will ~immediately need to be replaced when implementing the “add a new Zstd variant” feature; there’s no clean way to represent that.

We could hack that with ListOperation: ListOpAdd, AddDigest: theOldDigestToConvert; that would be ugly, but it would work once for the “add a Zstd variant” feature. But then someone may want an “add a gzip variant”… and we are missing a field where to recordwhat kind of modification we should make.

So I think it’s cleaner to add a separate struct immediately, like the original version of #1883 did e.g. in 941cd13 .

copy/multiple.go Outdated Show resolved Hide resolved
@flouthoc
Copy link
Contributor Author

flouthoc commented Jun 5, 2023

@mtrmac How about new struct could look like this ?

type addOperation int

const (
	addop_compress_gzip addOperation = iota
	addop_compress_zstd
)

type instanceCopy struct {
        instance internal.ListEdit
        addOp addOperation
}

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 5, 2023

  • That structure wouldn’t allow a non-converting copy that just copies layers as is
  • I don’t know what prepareEditList would fill in for most of the fields of instance.

I was thinking vaguely

/* enum instanceCopyKind {
   instanceCopyCopy // a single copy, replacing the original
   instanceCopyClone // creating a new copy of the original, not replacing it
} */

type instanceCopy {
   op instanceCopyKind
   sourceDigest // digest in the source == instance ID
   // otherCopyOptions: for now maybe just
   forceZstd bool
}

That’s assuming that after we do an instance copy, we have some other code to determine that we need to adjust the Zstd annotation, and that the annotation update doesn’t need to be represented here.

Maybe we don’t need a new instanceCopyKind enum (for now? at all?), just a clone bool (or createCloneInstance?) would be sufficient.

@flouthoc flouthoc force-pushed the split-what-and-how branch 2 times, most recently from 0e0cd7d to 8ec2734 Compare June 5, 2023 15:43
copy/multiple.go Outdated Show resolved Hide resolved
copy/multiple.go Outdated Show resolved Hide resolved
copy/multiple.go Outdated Show resolved Hide resolved
@flouthoc flouthoc force-pushed the split-what-and-how branch from 8ec2734 to 98b9147 Compare June 5, 2023 15:59
@flouthoc flouthoc requested a review from mtrmac June 5, 2023 16:56
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! A few more cleanups, please.

copy/multiple.go Outdated Show resolved Hide resolved
copy/multiple.go Outdated Show resolved Hide resolved
copy/multiple.go Outdated Show resolved Hide resolved
copy/multiple.go Outdated Show resolved Hide resolved
copy/multiple_test.go Outdated Show resolved Hide resolved
copy/multiple_test.go Outdated Show resolved Hide resolved
copy/multiple_test.go Outdated Show resolved Hide resolved
copy/multiple_test.go Outdated Show resolved Hide resolved
copy/multiple_test.go Outdated Show resolved Hide resolved
@flouthoc flouthoc force-pushed the split-what-and-how branch from 98b9147 to bd6a6ae Compare June 6, 2023 15:48
@flouthoc flouthoc requested a review from mtrmac June 6, 2023 15:52
@flouthoc
Copy link
Contributor Author

flouthoc commented Jun 6, 2023

@mtrmac PTAL again :)

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM.

@flouthoc
Copy link
Contributor Author

flouthoc commented Jun 6, 2023

@mtrmac @giuseppe @rhatdan PTAL

Following PR splits `copyMultipleImages` into a new function called
`prepareCopyInstances` which returns a list of `instanceCopy`.

`prepareCopyInstances` does that in these two steps

* How to copy via instanceCopyKind
* What to copy by the content of the returned list.

See newly added unit test to see how it is used.

Signed-off-by: Aditya R <[email protected]>
@mtrmac mtrmac force-pushed the split-what-and-how branch from bd6a6ae to 286f012 Compare June 6, 2023 16:19
@mtrmac mtrmac merged commit 3650b37 into containers:main Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants