Read insecure annotation on an image stream when import-image is invoked#8456
Read insecure annotation on an image stream when import-image is invoked#8456openshift-bot merged 2 commits intoopenshift:masterfrom
Conversation
pkg/cmd/cli/cmd/importimage.go
Outdated
| Spec: imageapi.ImageStreamImportSpec{Import: true}, | ||
| } | ||
| insecureAnnotation := stream.Annotations[imageapi.InsecureRepositoryAnnotation] | ||
| insecure := o.Insecure || insecureAnnotation == "true" |
There was a problem hiding this comment.
If a user passes oc import-image --insecure=false, should that take priority over the image stream? Why or why not?
There was a problem hiding this comment.
For why not - no opinions. For why yes - because that's what we do everywhere else, the flag has higher precedence than what's already there. I'll prepare an update to that...
|
@smarterclayton added the precedence you've asked for, ptal. |
pkg/cmd/cli/cmd/importimage.go
Outdated
| cmd.Flags().BoolVar(&opts.Confirm, "confirm", false, "If true, allow the image stream import location to be set or changed") | ||
| cmd.Flags().BoolVar(&opts.All, "all", false, "If true, import all tags from the provided source on creation or if --from is specified") | ||
| cmd.Flags().BoolVar(&opts.Insecure, "insecure", false, "If true, allow importing from registries that have invalid HTTPS certificates or are hosted via HTTP") | ||
| opts.Insecure = cmd.Flags().Bool("insecure", false, "If true, allow importing from registries that have invalid HTTPS certificates or are hosted via HTTP") |
There was a problem hiding this comment.
why use this syntax if you're checking later if the flag was changed and setting it to nil? You're effectively going to get the same thing as .BoolVar(opts.Insecure, "insecure", false,...) right?
There was a problem hiding this comment.
Yeah, you need to be checking whether the flag is set in Complete() and resetting it (or create a new boolean to track the user provided value).
There was a problem hiding this comment.
Just for clarity, that syntax is to prevent those NPE I'm getting right now in tests ;)
soltysh
force-pushed
the
issue8452
branch
2 times, most recently
from
19054c9 to
f4019f1
Compare
|
@pweil- @smarterclayton comments addressed. ptal |
|
LGTM - you're getting an error in travis from this bool though |
|
Needs update |
|
@smarterclayton @pweil- updated |
|
@smarterclayton last call and if I don't hear from you I assume you're ok with merging this as is. |
|
Paul had an outstanding comment |
|
That comment is addressed within the flag description and inside the code where the actual assignment happens. |
|
Lgtm [merge]
|
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3491/) (Image: devenv-rhel7_4064) |
|
Evaluated for origin merge up to 4d679ac |
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin test up to 4d679ac |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3491/) |
4 participants
Fixes #8452.
@pweil- @smarterclayton ptal