Skip to content

Conversation

@mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Aug 24, 2016

As we’ve discussed several times, this moves the core of skopeo copy into the library.

Depends on way too much pending work, I will rebase as necessary.

@mtrmac
Copy link
Collaborator Author

mtrmac commented Aug 31, 2016

Rebased to depend only on #64.

@runcom
Copy link
Member

runcom commented Aug 31, 2016

I'm reviewing code in the last commit 15a9bb7

source io.Reader
digest hash.Hash
expectedDigest []byte
failureIndicator *bool
Copy link
Member

Choose a reason for hiding this comment

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

just fail? I believe it's clear that it's indicating that by the bool (and pointer)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That reads wrong to me, I read verbs as method names.

The lazy way to deal with this is to say that this has not been modified from skopeo’s copy but that’s a poor excuse :)

Actually, this does not need to be a pointer at all, simply a bool validationFailed.

Still, for consistency I’d prefer moving the code as close to unmodified as possible, and I promise to fix that separately.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#65

@runcom
Copy link
Member

runcom commented Aug 31, 2016

LGTM

(after questions)

Approved with PullApprove

This uses code from projectatomic/skopeo, only stripping its CLI
handling.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac
Copy link
Collaborator Author

mtrmac commented Aug 31, 2016

👍

Approved with PullApprove

@mtrmac mtrmac merged commit 1743594 into containers:master Aug 31, 2016
@mtrmac mtrmac deleted the copy branch August 31, 2016 20:32
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