Skip to content

Conversation

@dvob
Copy link
Contributor

@dvob dvob commented Jul 16, 2020

Add platform option to crane copy and digest. If the remote reference is an index/list it only copies the image which matches the specified platform.

This would be a possible solution for #741

@dvob dvob force-pushed the crane-platform-option branch 2 times, most recently from 40a6738 to a141030 Compare July 16, 2020 19:45
@dvob dvob force-pushed the crane-platform-option branch 2 times, most recently from 1e2b4e7 to 4f5579f Compare July 16, 2020 20:53
@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2020

Codecov Report

Merging #742 into master will decrease coverage by 0.40%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #742      +/-   ##
==========================================
- Coverage   79.39%   78.99%   -0.41%     
==========================================
  Files         102      102              
  Lines        4683     4717      +34     
==========================================
+ Hits         3718     3726       +8     
- Misses        534      551      +17     
- Partials      431      440       +9     
Impacted Files Coverage Δ
pkg/crane/options.go 76.47% <0.00%> (-23.53%) ⬇️
pkg/crane/copy.go 51.11% <16.66%> (-2.55%) ⬇️
pkg/crane/manifest.go 47.05% <53.33%> (-52.95%) ⬇️
pkg/crane/digest.go 56.52% <61.90%> (-43.48%) ⬇️
pkg/crane/get.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72597da...92e4497. Read the comment docs.

@google google deleted a comment from jonjohnsonjr Jul 16, 2020
@dvob
Copy link
Contributor Author

dvob commented Jul 16, 2020

I moved the logic to the Digest and Manifest function. Does not look super nice, but is probably easier to understand than the logic I used before with the interface.

return nil, err
}
return desc.Manifest, nil
return img.RawManifest()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will still fail for schema 1 images, which we aren't able to satisfy the v1.Image interface for. If we keep the behavior the same here, but only try to resolve to an image when platform is specified, I think we can keep everything working, i.e.:

if o.platform != nil {
  img, err := desc.Image()
  if err != nil {
    return nil, err
  }
  return img.RawManifest()
}

return desc.Manifest, nil

Sorry for all the back and forth on this, I really appreciate your contribution!

@dvob dvob force-pushed the crane-platform-option branch from 47be60c to 213c3d9 Compare October 20, 2020 10:06
@codecov-io
Copy link

codecov-io commented Oct 20, 2020

Codecov Report

Merging #742 into master will decrease coverage by 0.38%.
The diff coverage is 11.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #742      +/-   ##
==========================================
- Coverage   75.94%   75.55%   -0.39%     
==========================================
  Files         102      102              
  Lines        4144     4169      +25     
==========================================
+ Hits         3147     3150       +3     
- Misses        546      565      +19     
- Partials      451      454       +3     
Impacted Files Coverage Δ
pkg/crane/options.go 75.00% <0.00%> (-25.00%) ⬇️
pkg/crane/digest.go 31.25% <8.33%> (-68.75%) ⬇️
pkg/crane/copy.go 48.78% <16.66%> (-2.58%) ⬇️
pkg/crane/manifest.go 50.00% <16.66%> (-50.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77421d5...213c3d9. Read the comment docs.

@dvob
Copy link
Contributor Author

dvob commented Oct 20, 2020

Tried to change the logic according to your points in the last review, squashed the commits and rebased them on master.
Since in the meantime crane.Digest was changed and now only uses HEAD. I had to change the logic that it still gets the manifest if a platform is specified. Or is there another solution?

@jonjohnsonjr
Copy link
Collaborator

I had to change the logic that it still gets the manifest if a platform is specified. Or is there another solution?

I think what you've done is the best we can do.

Copy link
Collaborator

@jonjohnsonjr jonjohnsonjr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

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.

4 participants