Skip to content

Conversation

jmrodri
Copy link
Member

@jmrodri jmrodri commented May 7, 2020

No description provided.

@jmrodri
Copy link
Member Author

jmrodri commented May 8, 2020

Copy link
Member

@kevinrizza kevinrizza left a comment

Choose a reason for hiding this comment

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

Had a couple questions for the most part. Mostly this looks like it's in decent shape!


1. if image doesn't have bash, we can't modify the entrypoint
1. if opm is building index images without bash, then there will be no index
images we support
Copy link
Member

Choose a reason for hiding this comment

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

If we want to support redhat supported indexes, we should also check to see what their base image is. I know they are definitely doing some replacement there. @gallettilance

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

Looking really good. A few comments.

* If `--index-image` is provided, use the provided value
* `[--namespace=]` is an optional flag that specifies the namespace to install
the operator. It will default to the `KUBECONFIG` context.
* `[--install-mode=]` is an optional flag that specifies the
Copy link
Member

Choose a reason for hiding this comment

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

The installModes are already defined by the bundle. I'm assuming this flag is for determining what OperatorGroup to generate - in that case, all we need is a --target-namespaces=[] arg taking the set of namespaces to watch (which matches the terminology in the OperatorGroup)

Copy link
Member

@shawn-hurley shawn-hurley May 11, 2020

Choose a reason for hiding this comment

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

We want someone to select the install mode to test not interface with the operator group I think was the thinking

Copy link
Member

@ecordell ecordell May 11, 2020

Choose a reason for hiding this comment

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

My point is that flag may be confusing. The flag is called installMode and the values are installMode values, but it does not set the installMode on the CSV (it can't, because that is already set in the bundle, and this command doesn't build the bundle).

What does this flag actually do?

Copy link
Member

Choose a reason for hiding this comment

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

A user can set --install-mode=InstallModeType[=nsY]. If a bundle's CSV supports that mode in installModes by comparing type strings, run bundle will either create an OperatorGroup in some namespace targeting nsY, or use an existing and compatible OperatorGroup.

I see where this can create confusion: a supported mode in installModes is only a statement about runtime availability, whereas --install-mode mixes in OperatorGroup semantics.

I don't think it makes sense to pollute the CLI with two flags, one for installMode and another for namespaces to target/watch. We definitely need the latter, and we want the former so users see a mapping between the way they're testing operator deployment and what their CSV/bundle encodes, even though we could infer mode from passed namespaces.

Copy link
Member

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

The UX for this needs to focus on users who will eventually use our pipelines. We need to match all the choices for the index they are making.

Copy link
Member

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 18, 2020
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants