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

Add insecure flag to push and pull #634

Merged
merged 4 commits into from
Dec 18, 2019

Conversation

erbesharat
Copy link
Contributor

@erbesharat erbesharat commented Dec 15, 2019

What this PR does / why we need it:
Adds the ability to push and pull from insecure registries to the crane cmd.
The flag also had to be added to a few other subcommands like export, append and etc.

Which issue this PR fixes:
fixes #633

Special notes for your reviewer:
I wasn't sure if it's ok to change the handler map in the validate subcommand without any prior discussions, so I just added the options argument to makeTarball as a temporary fix so we don't have failing tests.
Let me know if it's ok to change the handler map and also what do you think is a good approach to change it.
https://github.com/google/go-containerregistry/pull/634/files#diff-67c9d71faf9a3b5faa49b6f666170f54R72

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@erbesharat
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@codecov-io
Copy link

codecov-io commented Dec 15, 2019

Codecov Report

Merging #634 into master will increase coverage by 0.24%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #634      +/-   ##
==========================================
+ Coverage   76.29%   76.53%   +0.24%     
==========================================
  Files         101      101              
  Lines        4412     4483      +71     
==========================================
+ Hits         3366     3431      +65     
  Misses        636      636              
- Partials      410      416       +6
Impacted Files Coverage Δ
pkg/crane/pull.go 87.5% <100%> (ø) ⬆️
pkg/crane/push.go 100% <100%> (ø) ⬆️
pkg/v1/remote/catalog.go 67.64% <0%> (-0.36%) ⬇️
pkg/internal/retry/retry.go 100% <0%> (ø) ⬆️
pkg/internal/compare/index.go 76.47% <0%> (+5.88%) ⬆️
pkg/v1/remote/list.go 76.66% <0%> (+11.44%) ⬆️
pkg/internal/compare/image.go 76.47% <0%> (+11.76%) ⬆️
pkg/crane/catalog.go 100% <0%> (+37.5%) ⬆️

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 2f24255...9e41346. Read the comment docs.

@erbesharat erbesharat marked this pull request as ready for review December 15, 2019 13:01
Copy link
Collaborator

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

Thanks for this change! Small comment otherwise looks good.

pkg/crane/pull.go Outdated Show resolved Hide resolved
pkg/crane/push.go Outdated Show resolved Hide resolved
@jonjohnsonjr
Copy link
Collaborator

You'll want to run ./hack/update-code-gen.sh to re-generate the docs, otherwise lgtm.

I wasn't sure if it's ok to change the handler map in the validate subcommand without any prior discussions, so I just added the options argument to makeTarball as a temporary fix so we don't have failing tests.

I don't see any problem with what you've done here, that looks like what I'd have done.

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.

Force Crane to use http instead of https
5 participants