-
Notifications
You must be signed in to change notification settings - Fork 377
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
Bug 1827544: provide better defaults for oc adm catalog build #423
Bug 1827544: provide better defaults for oc adm catalog build #423
Conversation
3d68e5b
to
9bb422d
Compare
@ecordell: This pull request references Bugzilla bug 1827544, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/hold I'm going to change this to grab the base image from a target ocp release |
9bb422d
to
f144d86
Compare
/hold cancel |
f144d86
to
163a43c
Compare
@ecordell: This pull request references Bugzilla bug 1827544, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
163a43c
to
20918a2
Compare
44fe2be
to
fbe679e
Compare
/retest |
fbe679e
to
ca01c37
Compare
/retest gce error: lookupDiskImageSources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why we need a new flag, when --from
and --release-image
exclude each other? Or am I misreading the intention here, but the code clearly gives precedence to --from
if both are specified. Which means you should be able to pass whatever you wanted through --from
, no?
Yes, you can pass whatever you want as a base image. Typically you'll want to pick the base image from a release, but there are cases where that's not what you want (you might want to pick a newer image to fix a bug, or you might want be targeting an upstream cluster, or you might be QE testing different variants). So the order is: if you've told us the base image ( |
If that's the case, I still think all of that should be baked in one flag, having more is just confusing for user as in which one should I use, what happens if both are set, etc. Having a single gives simple information - that's the one we use, or we default to a cluster. |
ca01c37
to
62bba08
Compare
- will try to pick a base image from a release - will default to linux/amd64 for the base image arch when building
141a930
to
c5b9ac6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more nits, but I like this, thx!
/approve
pkg/cli/admin/catalog/build.go
Outdated
@@ -26,16 +29,36 @@ var ( | |||
|
|||
Extracts the contents of a collection of operator manifests to disk, and builds them into | |||
an operator registry catalog image. | |||
|
|||
The base image used for the catalog should match the target version of ocp. This can be set manually with | |||
the '--from' flag, or it can be inferred from an ocp release image via '--release-image', or directly from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You left --release-image
mentioned here.
pkg/cli/admin/catalog/build.go
Outdated
@@ -113,7 +184,7 @@ func NewBuildImage(streams genericclioptions.IOStreams) *cobra.Command { | |||
o.FilterOptions.Bind(flags) | |||
o.ParallelOptions.Bind(flags) | |||
|
|||
flags.StringVar(&o.From, "from", o.From, "The image to use as a base.") | |||
flags.StringVar(&o.From, "from", o.From, "The image to use as a base. This can be omitted if `--release-image` is specified or if oc is already logged into the target cluster.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again --release-image
being mentioned.
this removes the release-image flag and instead detects whether the targeted image is a release image
c5b9ac6
to
7d530ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ecordell, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
7 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@ecordell: All pull requests linked via external trackers have merged: openshift/oc#423. Bugzilla bug 1827544 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
--from