Bug 1826646: shows alert in eventsources form if knative service is not present in namespace #5144

invincibleJai · 2020-04-22T08:44:14Z

Fixes:
https://issues.redhat.com/browse/ODC-3439

Analysis / Root cause:
In Console EventSources form supports only sink to knative Service and in case of no associated service for that form will be never get enabled and user has not much information apart from form field

Solution Description:
Adds an Alert in EventSources form which would be shown if there are no knative service in that namespace

Gif/Image:

Tests:

Browser conformance:

Chrome
Firefox
Safari
Edge

openshift-ci-robot · 2020-04-22T08:44:23Z

@invincibleJai: This pull request references Bugzilla bug 1826646, 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

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1826646: shows alert in eventsources form if knative service is not present in namespace

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.

invincibleJai · 2020-04-22T10:47:35Z

@serenamarie125 PTAL , needed your review

/kind bug

sahil143

/lgtm

verifed changes locally

sahil143 · 2020-04-22T11:58:06Z

/lgtm cancel

sahil143

@invincibleJai spacing between heading the alert doesn't seem right.

UX design

Please confirm with UX.

invincibleJai · 2020-04-22T12:24:10Z

@sahil143 updated , PTAL

serenamarie125

Thanks for making these changes & collaborating on the design approach @invincibleJai ! Approved by UX

rohitkrai03 · 2020-04-22T19:50:10Z

frontend/packages/knative-plugin/src/components/add/EventSource.tsx

+          {...props}
+          namespace={namespace}
+          projects={projects}
+          showSinkAlert={service?.loaded && !service?.data?.length}


@invincibleJai Why not render the alert here in this component only? The alert doesn't seem to be related to the form in any way.

christianvogt · 2020-04-22T20:16:57Z

frontend/packages/knative-plugin/src/components/add/EventSourceForm.tsx

+    {showSinkAlert && (
+      <Alert variant="default" title="Event Source can not be created" isInline>
+        An event source must sink to Knative Service, but no Knative Service exist in this project
+      </Alert>
+    )}


Move to separate component and have it do it's own fetching of the knative service to show the alert.

Right now this change is spread across 3 different components when it should be isolated to a single component.

Also try to use the firehose hook.

Have fixed it and refactored code Projects are also fetched with useK8sWatchResource

… namespace

openshift-ci-robot · 2020-04-23T08:26:02Z

@invincibleJai: This pull request references Bugzilla bug 1826646, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1826646: shows alert in eventsources form if knative service is not present in namespace

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.

frontend/packages/knative-plugin/src/components/add/ShowResourceAlert.tsx

christianvogt · 2020-04-23T19:21:12Z

frontend/packages/knative-plugin/src/components/add/EventSourcePage.tsx


 type EventSourcePageProps = RouteComponentProps<{ ns?: string }>;

 const EventSourcePage: React.FC<EventSourcePageProps> = ({ match, location }) => {
  const namespace = match.params.ns;
  const searchParams = new URLSearchParams(location.search);
-  const resources = [{ kind: ProjectModel.kind, prop: ProjectModel.id, isList: true }];
+  const knServiceResource = { ...knativeServingResourcesServices(namespace)[0], limit: 1 };


I think you should still move this into NoKnativeServiceAlert and pass the namespace as a prop.
There's no need for the parent to have to deal with passing along a firehose query because then there's no constrain on the resource or limit.
If you just pass the namespace, internal to NoKnativeServiceAlert it knows exactly what to look for.

christianvogt · 2020-04-23T20:27:37Z

/lgtm

openshift-ci-robot · 2020-04-23T20:27:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, invincibleJai, serenamarie125

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/knative-plugin/OWNERS~~ [christianvogt,invincibleJai]

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 · 2020-04-23T23:23:48Z

@invincibleJai: All pull requests linked via external trackers have merged: openshift/console#5144. Bugzilla bug 1826646 has been moved to the MODIFIED state.

In response to this:

Bug 1826646: shows alert in eventsources form if knative service is not present in namespace

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-ci-robot added bugzilla/unspecified bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Apr 22, 2020

openshift-ci-robot added the component/knative Related to knative-plugin label Apr 22, 2020

openshift-ci-robot requested review from christianvogt and divyanshiGupta April 22, 2020 08:44

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2020

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 22, 2020

sahil143 approved these changes Apr 22, 2020

View reviewed changes

openshift-ci-robot assigned sahil143 Apr 22, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2020

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2020

sahil143 suggested changes Apr 22, 2020

View reviewed changes

invincibleJai force-pushed the fix-alert-for-sink branch from 1f9b242 to 6effaf2 Compare April 22, 2020 12:23

serenamarie125 approved these changes Apr 22, 2020

View reviewed changes

rohitkrai03 suggested changes Apr 22, 2020

View reviewed changes

christianvogt reviewed Apr 22, 2020

View reviewed changes

invincibleJai force-pushed the fix-alert-for-sink branch from 6effaf2 to b86bb9d Compare April 23, 2020 07:58

shows alert in eventsources form if knative service is not present in…

701e923

… namespace

invincibleJai force-pushed the fix-alert-for-sink branch from b86bb9d to 701e923 Compare April 23, 2020 08:24

sahil143 reviewed Apr 23, 2020

View reviewed changes

frontend/packages/knative-plugin/src/components/add/ShowResourceAlert.tsx Outdated Show resolved Hide resolved

invincibleJai force-pushed the fix-alert-for-sink branch from f5708d7 to a52717d Compare April 23, 2020 18:30

christianvogt reviewed Apr 23, 2020

View reviewed changes

made use of useK8sWatchResource to fetch Projects

81a3c4f

invincibleJai force-pushed the fix-alert-for-sink branch from a52717d to 81a3c4f Compare April 23, 2020 20:16

openshift-ci-robot assigned christianvogt Apr 23, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2020

openshift-merge-robot merged commit 323948e into openshift:master Apr 23, 2020

invincibleJai mentioned this pull request Apr 24, 2020

Bug 1827356: fix event source form for single event source #5175

Merged

4 tasks

spadgett added this to the v4.5 milestone Apr 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1826646: shows alert in eventsources form if knative service is not present in namespace #5144

Bug 1826646: shows alert in eventsources form if knative service is not present in namespace #5144

invincibleJai commented Apr 22, 2020 •

edited

openshift-ci-robot commented Apr 22, 2020

invincibleJai commented Apr 22, 2020

sahil143 left a comment

sahil143 commented Apr 22, 2020

sahil143 left a comment

invincibleJai commented Apr 22, 2020

serenamarie125 left a comment

rohitkrai03 Apr 22, 2020 •

edited

invincibleJai Apr 23, 2020

christianvogt Apr 22, 2020

invincibleJai Apr 23, 2020

openshift-ci-robot commented Apr 23, 2020

christianvogt Apr 23, 2020

christianvogt commented Apr 23, 2020

openshift-ci-robot commented Apr 23, 2020

openshift-ci-robot commented Apr 23, 2020

Bug 1826646: shows alert in eventsources form if knative service is not present in namespace #5144

Bug 1826646: shows alert in eventsources form if knative service is not present in namespace #5144

Conversation

invincibleJai commented Apr 22, 2020 • edited

openshift-ci-robot commented Apr 22, 2020

invincibleJai commented Apr 22, 2020

sahil143 left a comment

Choose a reason for hiding this comment

sahil143 commented Apr 22, 2020

sahil143 left a comment

Choose a reason for hiding this comment

invincibleJai commented Apr 22, 2020

serenamarie125 left a comment

Choose a reason for hiding this comment

rohitkrai03 Apr 22, 2020 • edited

Choose a reason for hiding this comment

invincibleJai Apr 23, 2020

Choose a reason for hiding this comment

christianvogt Apr 22, 2020

Choose a reason for hiding this comment

invincibleJai Apr 23, 2020

Choose a reason for hiding this comment

openshift-ci-robot commented Apr 23, 2020

christianvogt Apr 23, 2020

Choose a reason for hiding this comment

christianvogt commented Apr 23, 2020

openshift-ci-robot commented Apr 23, 2020

openshift-ci-robot commented Apr 23, 2020

invincibleJai commented Apr 22, 2020 •

edited

rohitkrai03 Apr 22, 2020 •

edited