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

Subscribe to Single Namespace Operator #1172

Merged
merged 1 commit into from
Feb 22, 2019

Conversation

alecmerdler
Copy link
Contributor

@alecmerdler alecmerdler commented Feb 6, 2019

Description

Some Operators in the Marketplace do not support watching their custom resources at a cluster-wide level. OLM only ships with an OperatorGroup for cluster-wide Operators. Here we modify the Marketplace and OLM subscription workflows to support single-namespace Operators by creating a new OperatorGroup in the selected namespace. This also removes the existing OperatorGroup dropdown for cluster-wide Operators, because there will only ever be one "global" OperatorGroup per OLM installation.

Also adds full Marketplace UI end-to-end test for installing the etcd Operator.

Screenshots

"Create Operator Subscription" (single namespace):
screenshot_20190208_161832

"Create Operator Subscription" (all namespaces):
screenshot_20190205_144550

"Create Operator Subscription" (w/ tooltip):
screen shot 2019-02-08 at 12 53 40 pm

"Create Operator Subscription" view (Subscription exists in namespace):
screenshot_20190208_143058

Addresses https://jira.coreos.com/browse/ALM-894
Blocked by operator-framework/operator-lifecycle-manager#697

@alecmerdler alecmerdler changed the title Subscribe to Single Namespace Operator [WIP] Subscribe to Single Namespace Operator Feb 6, 2019
@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 6, 2019
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 6, 2019
@robszumski
Copy link
Contributor

Can you nest the dropdown under the radio label, so that the namespace selection feels like it is part of the selection?

The mock also has the dropdown always visible, even when disabled.

@alecmerdler
Copy link
Contributor Author

@robszumski We don't currently have a way to disable the dropdown, and we have an existing pattern when creating a RoleBinding that hides the namespace selector when the subject is a ServiceAccount.

@spadgett
Copy link
Member

spadgett commented Feb 6, 2019

cc @jwforres

@jwforres
Copy link
Member

jwforres commented Feb 6, 2019

Agree with rob on at least nesting it under the radio visually with some left margin.

@robszumski
Copy link
Contributor

Are we actually looking at a Subscription for checking if it's installed? Or looking for the presence of the real/copied CSV? Is your error message accurate?

@robszumski We don't currently have a way to disable the dropdown, and we have an existing pattern when creating a RoleBinding that hides the namespace selector when the subject is a ServiceAccount.

Ok, in the interest of time that is ok with me.

@serenamarie125
Copy link
Contributor

I'm not crazy about showing a radio button group with a single option (update channel) or include a disabled option. I think this is something we should stay away from in the UI.

@serenamarie125
Copy link
Contributor

Can we add explanatory text to let the users know why some operators will/not have both options?

@serenamarie125
Copy link
Contributor

Based on the mocks Rob shared, i think we can come to this screen from an Operator Hub which is scoped to a specific namespace. If that's the case, could we simplify it even more and not include an installation mode?

@alecmerdler
Copy link
Contributor Author

I'm not crazy about showing a radio button group with a single option (update channel) or include a disabled option. I think this is something we should stay away from in the UI.

This was dictated in the original design, not part of this PR.

Can we add explanatory text to let the users know why some operators will/not have both options?

Sure. What specific text should we use that doesn't "confuse" the user or expose the concept of OperatorGroups?

Based on the mocks Rob shared, i think we can come to this screen from an Operator Hub which is scoped to a specific namespace. If that's the case, could we simplify it even more and not include an installation mode?

The goal is that all Operators will support multiple installation modes. Without this option, we don't expose the installation mode anywhere, so admins won't know if this Operator watches all-namespaces, a single namepace, or a set of namespaces.

@serenamarie125
Copy link
Contributor

@alecmerdler thanks for the replies. I’m fine with merging but would like to revisit the design to see how we can improve

@alecmerdler alecmerdler force-pushed the ALM-894 branch 4 times, most recently from ea91130 to 5dfed30 Compare February 7, 2019 21:16
@robszumski
Copy link
Contributor

I'm not crazy about showing a radio button group with a single option (update channel) or include a disabled option. I think this is something we should stay away from in the UI.

@serenamarie125 These are per-CSV and I was a fan of showing this because the channel might mean something to you, eg. seeing only alpha might mean you might avoid installing it. But it's dynamic per the Operator author.

@alecmerdler alecmerdler force-pushed the ALM-894 branch 2 times, most recently from e4feb72 to 55ac82b Compare February 8, 2019 18:23
@alecmerdler alecmerdler changed the title [WIP] Subscribe to Single Namespace Operator Subscribe to Single Namespace Operator Feb 8, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 8, 2019
@openshift openshift deleted a comment from robszumski Feb 8, 2019
@spadgett
Copy link
Member

/retest

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2019
@spadgett
Copy link
Member

Tagging based on previous reviews

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2019
@spadgett
Copy link
Member

I0222 18:46:59.323243      30 log.go:172] Unsolicited response received on idle HTTP channel starting with "HTTP/1.0 408 Request Time-out\r\nCache-Control: no-cache\r\nConnection: close\r\nContent-Type: text/html\r\n\r\n<html><body><h1>408 Request Time-out</h1>\nYour browser didn't send a complete request in time.\n</body></html>\n"; err=<nil>
error: unable to parse image registry.svc.ci.openshift.org/ci-op-hf3fd5xf/stable@sha256:23c80b26ced817a5289acda107a6ba906b0f39f5d8f17246c41c8b6ca47ff515: cannot retrieve image configuration for manifest sha256:23c80b26ced817a5289acda107a6ba906b0f39f5d8f17246c41c8b6ca47ff515: received unexpected HTTP status: 504 Gateway Time-out

/retest

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2019
test-prow-e2e.sh Outdated Show resolved Hide resolved
@alecmerdler alecmerdler force-pushed the ALM-894 branch 2 times, most recently from eb1889b to 4d34d79 Compare February 22, 2019 19:58
@spadgett
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2019
@spadgett
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2019
@spadgett
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2019
@jwforres
Copy link
Member

/override console-e2e

alec has confirmed these tests pass for him locally

@jwforres
Copy link
Member

/override ci/prow/console-e2e

not sure which it is im supposed to tell it here :)

@openshift-ci-robot
Copy link
Contributor

@jwforres: Overrode contexts on behalf of jwforres: ci/prow/console-e2e

In response to this:

/override ci/prow/console-e2e

not sure which it is im supposed to tell it here :)

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.

@openshift-merge-robot openshift-merge-robot merged commit 721a623 into openshift:master Feb 22, 2019
@alecmerdler alecmerdler deleted the ALM-894 branch February 22, 2019 21:56
@openshift-ci-robot
Copy link
Contributor

@alecmerdler: The following test failed for commit 09df3ba, say /retest to rerun them:

Test name Details Rerun command
ci/prow/console-e2e link /test console-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants