Bug 1794825: Operator-defined namespace that requests monitoring should fully warn user of implications of enabling

[jhadvig]

Adding a warning alert to emphasise to the users what the implications of allowing any workload to contribute metrics are. The wording is taken from the bug's Doc Text.

Here I'm not really sure if it does make sense since the warning alert will be shown in case only when openshift-* namespace is selected as Installed Namespace. Is that on purpose

Also the Doc Text is pointing to 4.2 docs. Should we use 4.3 instead?

Last thing, wanted to ask if there shouldn’t also be a checkbox for enabling the monitoring when user picks from an existing namespace and the operator has openshift.io/cluster-monitoring annotation defined. Or there was an agreement that the checkbox will be only shown when using the recommended namespace ?

@shawn-hurley @itsptk could you help here?

Screen:

/assign @spadgett

[openshift-ci-robot]

@jhadvig: This pull request references Bugzilla bug 1794825, 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.

In response to this:

Bug 1794825: Operator-defined namespace that requests monitoring should fully warn user of implications of enabling

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.

[spadgett]

Here I'm not really sure if it does make sense since the warning alert will be shown in case only when openshift-* namespace is selected as Installed Namespace. Is that on purpose

Yes, you can't currently enable monitoring for non-openshift namespaces.

Also the Doc Text is pointing to 4.2 docs. Should we use 4.3 instead?

Using openshiftHelpBase will point to the right version of the docs for the cluster.

Last thing, wanted to ask if there shouldn’t also be a checkbox for enabling the monitoring when user picks from an existing namespace and the operator has openshift.io/cluster-monitoring annotation defined. Or there was an agreement that the checkbox will be only shown when using the recommended namespace ?

We tried to keep it simple for 4.4 to get the basic implementation done.

[jhadvig]

@spadgett comments addressed.

Screen:

[itsptk]

@jhadvig I was expecting this new text to replace the existing "Note: Enabling monitoring will allow any operator or workload running on this namespace to contribute metrics to the cluster metric set." I don't think we need both and not sure we need to use the inline alert control.

If we really wanted to use the alert I would say remove the existing "Note:" text and just show the inline alert when the user checks to Enable monitoring.

[jhadvig]

@itsptk if we want to emphasise the consequences from enabling the monitoring I would keep the Alert instead of the small note.
@spadgett thoughts ?

[spadgett]

@bparees fyi

[spadgett]

@shawn-hurley Is it reasonable to remove the warning if the provider type is Red Hat? Console currently reads the opsrc-provider label on the PackageManifest.

If we can hide it for Red Hat operators, I would vote for the stronger warning. Agree w/ @itsptk that we shouldn't show it twice :)

[shawn-hurley]

I think that was agreed upon @spadgett

I am also a +1 on the stronger message and only showing one.

[itsptk]

I agree that if we aren't always going to show this alert (only when the provider isn't Red Hat) then the alert would be best and the Note: should be removed.

I am changing my mind and thinking that we should show the alert regardless if the box is checked or not, so the user is aware of the implications of checking it before they check it, and not just surprised with a warning for something the UI said was recommended.

[spadgett]

Suggested change

	<Alert
	{props.packageManifest.data[0].metadata.labels['opsrc-provider'] !== 'redhat' && <Alert

[kyoto]

FYI @openshift/openshift-team-monitoring

[lilic]

Thanks! 👍

[jhadvig]

@spadgett comment addressed. Screen:

PTAL

[itsptk]

There's definitely a lot going on in the UI in this scenario (namespace being created, requesting monitoring,) but I think it is worth being verbose so the user understands whats happening.

LGTM

[spadgett]

/test analyze

[spadgett]

/approve
/lgtm

Thanks @jhadvig

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhadvig, LiliC, spadgett

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:

• frontend/packages/operator-lifecycle-manager/OWNERS [jhadvig,spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci-robot]

@jhadvig: All pull requests linked via external trackers have merged. Bugzilla bug 1794825 has been moved to the MODIFIED state.

In response to this:

Bug 1794825: Operator-defined namespace that requests monitoring should fully warn user of implications of enabling

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.