adds support for creation of eventSources and form for CronJobSource #4748

invincibleJai · 2020-03-17T07:53:58Z

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

Analysis / Root cause:
User can't create EventSources via webConsole

Solution Description:
Enable support to create Event Sources via Add Flow and added form for CronJobSource

Screen shots / Gifs for design review:

Unit test coverage report:

Browser conformance:

Chrome
Firefox
Safari
Edge

cc @openshift/team-devconsole-ux @serenamarie125

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

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

frontend/packages/knative-plugin/src/components/add/event-sources/EventSourcesSelector.tsx

rohitkrai03 · 2020-03-17T19:20:12Z

frontend/packages/knative-plugin/src/components/add/event-sources/SinkSection.tsx

+        helperText="Select an Service to sink to."
+        isRequired
+      >
+        <Dropdown


Can we not use the ResourceDropdown component here? That way the component handles its own fetch logic and we do not need to fetch the resource in firehose in parent.

yeah , have used it this way to show resource badge as well i.e ksvc here as later sink will not be limited to knative service , then it'll be useful. WDYT?

Enhance the component to include the badges then?
Right now you have copied code from resource-dropdown

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

serenamarie125

hey @invincibleJai this is looking great, any chance you can include a non animated gif of the form. I'd like to review the text it's a bit fuzzy for me to see!

I think we can add a temporary icon for the Add page, and we can have a follow up PR to put that in.

I did notice that there don't seem to have Adv Options available. We are supposed to, right?

invincibleJai · 2020-03-19T02:46:01Z

@serenamarie125 here is screenshot and yes advance options will be there with resource limit and resource request. I'll be adding it as subsequent PRs (NOTE: CronJobSrc will be changed to Ping Source which isn't available in operstorHub but their spec are same).

serenamarie125 · 2020-03-19T02:48:39Z

I was curious about the help text! I'll add a design story for us to clarify these details, and we can follow up later. This looks great ! Thanks for info on the Adv Options

andrewballantyne · 2020-03-19T18:14:56Z

/kind feature

christianvogt · 2020-03-19T16:51:10Z

frontend/packages/dev-console/src/components/EmptyState.tsx

+                  navigateTo(e, `/event-source?preselected-ns=${activeNamespace}`)
+                }
+                href={`/event-source?preselected-ns=${activeNamespace}`}


Does preselected-ns even do anything? I see it being used for the deploy image route as well.
@rohitkrai03 thoughts?

not much which i saw, i added as was there for deploy image

If it's not required, please remove here and for the other link.

frontend/packages/dev-console/src/components/import/builder/SelectorCard.scss

christianvogt · 2020-03-19T18:29:53Z

frontend/packages/knative-plugin/src/components/add/event-sources/EventSourcesSelector.tsx

+  );
+  return (
+    <FormSection title="Type" fullWidth>
+      <div id="event-source-selector-field" className="odc-event-source-selector">


This should be a reusable component so we don't duplicate styles.
Maybe even a reusable formik field?

frontend/packages/knative-plugin/src/components/add/event-sources/SinkSection.tsx

christianvogt · 2020-03-19T19:07:59Z

frontend/packages/dev-console/src/components/import/builder/SelectorCard.tsx

  onChange: (name: string) => void;
 }

-const BuilderImageCard: React.FC<BuilderImageCardProps> = ({
+const SelectorCard: React.FC<SelectorCardProps> = ({


Would be useful as a reusable component along with the parent container. Maybe even a formik field.

yeah, parent has lot of code which are coupled as it's used in git import and catalog

Yeah, I like the idea of having a ImageSelectorField that wraps all the logic and gives a clean API to use. It was not a shared component as there was no other use case for it till now. I had a discussion with Jai around this today and we came to a solution that works for both use cases.

yeah , have updated the code PTAL :)

frontend/packages/knative-plugin/src/components/add/event-sources/SinkSection.tsx

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

christianvogt · 2020-03-20T21:32:06Z

...ckages/console-shared/src/components/formik-fields/item-selector-field/ItemSelectorField.tsx

+  title: string;
+  displayName: string;
+  iconUrl?: string;
+  [x: string]: any;


You shouldn't need this.

christianvogt · 2020-03-20T21:41:57Z

...nd/packages/console-shared/src/components/formik-fields/item-selector-field/SelectorCard.tsx

+        <img className="odc-selector-card__icon" src={iconUrl} alt={displayName} />
+      </CardHeader>
+      <CardBody>
+        <span className="odc-selector-card__title">{title}</span>


I know this is old code, but it's extremely odd that the display name is used as an image alt.
Perhaps rename to fullName.

While updating i came across this as i think it's referenced from annotations

const displayName = _.get(imageTag, ['annotations', 'openshift.io/display-name'], defaultName);

https://github.com/openshift/console/blob/master/frontend/packages/dev-console/src/utils/imagestream-utils.ts#L128

https://github.com/openshift/console/blob/master/frontend/packages/dev-console/src/utils/imagestream-utils.ts#L98-L102

i need to change across let me know as it may be confusing based on annotations name

...nd/packages/console-shared/src/components/formik-fields/item-selector-field/SelectorCard.tsx

christianvogt · 2020-03-20T21:42:41Z

...nd/packages/console-shared/src/components/formik-fields/item-selector-field/SelectorCard.tsx

+  title: string;
+  iconUrl: string;
+  name: string;
+  displayName: string;


Suggested change

displayName: string;

fullName?: string;

...nd/packages/console-shared/src/components/formik-fields/item-selector-field/SelectorCard.tsx

...ckages/console-shared/src/components/formik-fields/item-selector-field/ItemSelectorField.tsx

christianvogt · 2020-03-20T21:46:59Z

frontend/packages/dev-console/src/components/EmptyState.tsx

+                  navigateTo(e, `/event-source?preselected-ns=${activeNamespace}`)
+                }
+                href={`/event-source?preselected-ns=${activeNamespace}`}


If it's not required, please remove here and for the other link.

frontend/packages/knative-plugin/src/components/add/event-sources/CronJobSection.tsx

christianvogt · 2020-03-20T21:52:50Z

frontend/packages/knative-plugin/src/components/add/event-sources/SinkSection.tsx

+        helperText="Select an Service to sink to."
+        isRequired
+      >
+        <Dropdown


Enhance the component to include the badges then?
Right now you have copied code from resource-dropdown

christianvogt · 2020-03-23T19:31:01Z

frontend/packages/console-shared/src/components/dropdown/ResourceDropdown.tsx

+const DropdownItem: React.FC<DropdownItemProps> = ({ model, name }) => (
+  <span className="co-resource-item">
+    <span className="co-resource-icon--fixed-width">
+      <ResourceIcon kind={referenceForModel(model)} />
+    </span>
+    <span className="co-resource-item__resource-name">
+      <span className="co-resource-item__resource-api co-truncate show co-nowrap small">
+        {name}
+      </span>
+    </span>
+  </span>
+);


All these classes are part of resource-dropdown.
We should not be reusing classes like this because that component could change and will break this component.
If we keep this here we need a tech debt item, and a comment, to cover fixing this issue.

Have updated the classes, just used classes on PF i.e co-resource-item and common styles

christianvogt · 2020-03-23T19:35:19Z

...ckages/console-shared/src/components/formik-fields/item-selector-field/ItemSelectorField.tsx

-          <br />
-        </>
-      )}
+    <FormGroup fieldId={fieldId} helperTextInvalid={errorMessage} isValid={isValid} isRequired>


Missing optional label prop.
You may not use it now but as a re-usable field, it should be present.

christianvogt · 2020-03-23T19:40:12Z

frontend/packages/knative-plugin/src/components/add/event-sources/SinkSection.tsx

+          label="Knative Service"
+          resources={knativeServingResourcesServices(namespace)}
+          dataSelector={['metadata', 'name']}
+          key={namespace}


Do you specifically need a key to force unmounting here? Otherwise remove the key.

christianvogt · 2020-03-24T02:12:54Z

/lgtm
/approve

openshift-ci-robot · 2020-03-24T02:13:10Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/OWNERS~~ [christianvogt]

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 requested review from andrewballantyne and jeff-phillips-18 March 17, 2020 07:54

openshift-ci-robot added component/dev-console Related to dev-console component/knative Related to knative-plugin approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 17, 2020

invincibleJai force-pushed the feat-event-src-creation branch 4 times, most recently from e54036f to 0f93dcd Compare March 17, 2020 16:19

rohitkrai03 suggested changes Mar 17, 2020

View reviewed changes

invincibleJai force-pushed the feat-event-src-creation branch from b40a742 to 79e7d80 Compare March 18, 2020 16:24

serenamarie125 reviewed Mar 18, 2020

View reviewed changes

openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 19, 2020

christianvogt reviewed Mar 19, 2020

View reviewed changes

invincibleJai force-pushed the feat-event-src-creation branch from 79e7d80 to 4cb764f Compare March 20, 2020 07:35

karthikjeeyar reviewed Mar 20, 2020

View reviewed changes

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

openshift-ci-robot added component/shared Related to console-shared and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 20, 2020

invincibleJai force-pushed the feat-event-src-creation branch 2 times, most recently from 57b35e7 to 0add520 Compare March 20, 2020 14:55

christianvogt reviewed Mar 20, 2020

View reviewed changes

invincibleJai force-pushed the feat-event-src-creation branch 2 times, most recently from 2452ab7 to 532f1ea Compare March 23, 2020 15:10

christianvogt reviewed Mar 23, 2020

View reviewed changes

invincibleJai added 3 commits March 24, 2020 07:08

adds support for creation of eventSources and form for CronJobSource

83926d9

fixes review comments

63cd919

creates generic ItemSelectorField

77d5387

invincibleJai force-pushed the feat-event-src-creation branch from 532f1ea to f90c19d Compare March 24, 2020 01:58

extend resourceDropdown to showbadges and fixes review comments

3112220

invincibleJai force-pushed the feat-event-src-creation branch from f90c19d to 3112220 Compare March 24, 2020 02:03

openshift-ci-robot assigned christianvogt Mar 24, 2020

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

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

openshift-merge-robot merged commit 8871465 into openshift:master Mar 24, 2020

spadgett added this to the v4.5 milestone Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds support for creation of eventSources and form for CronJobSource #4748

adds support for creation of eventSources and form for CronJobSource #4748

invincibleJai commented Mar 17, 2020 •

edited

rohitkrai03 Mar 17, 2020

invincibleJai Mar 18, 2020 •

edited

christianvogt Mar 20, 2020

serenamarie125 left a comment

invincibleJai commented Mar 19, 2020

serenamarie125 commented Mar 19, 2020

andrewballantyne commented Mar 19, 2020

christianvogt Mar 19, 2020

invincibleJai Mar 19, 2020

christianvogt Mar 20, 2020

christianvogt Mar 19, 2020

christianvogt Mar 19, 2020

invincibleJai Mar 19, 2020

rohitkrai03 Mar 20, 2020

invincibleJai Mar 20, 2020

christianvogt Mar 20, 2020

christianvogt Mar 20, 2020

invincibleJai Mar 23, 2020 •

edited

christianvogt Mar 20, 2020

christianvogt Mar 20, 2020

christianvogt Mar 20, 2020

christianvogt Mar 23, 2020

invincibleJai Mar 24, 2020

christianvogt Mar 23, 2020

christianvogt Mar 23, 2020

christianvogt commented Mar 24, 2020

openshift-ci-robot commented Mar 24, 2020

adds support for creation of eventSources and form for CronJobSource #4748

adds support for creation of eventSources and form for CronJobSource #4748

Conversation

invincibleJai commented Mar 17, 2020 • edited

Choose a reason for hiding this comment

invincibleJai Mar 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serenamarie125 left a comment

Choose a reason for hiding this comment

invincibleJai commented Mar 19, 2020

serenamarie125 commented Mar 19, 2020

andrewballantyne commented Mar 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invincibleJai Mar 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt commented Mar 24, 2020

openshift-ci-robot commented Mar 24, 2020

invincibleJai commented Mar 17, 2020 •

edited

invincibleJai Mar 18, 2020 •

edited

invincibleJai Mar 23, 2020 •

edited