add non blocking pipeline creation flow #7241

karthikjeeyar · 2020-11-17T14:03:47Z

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

Analysis / Root cause:
As a user, I'd like to still be able to finish my add flow even if the Pipeline Run cannot be created for the added Pipeline.

Solution Description:
Removed the pipelinerun creation request from the promise collection and added it in a try/catch block seperately.

Screen shots / Gifs for design review:

Before:
Blocks the add flow redirection and shows the error.

After:
Does not block the add flow redirection and suppresses the pipelinerun creation error

Topology Sidebar

Unit test coverage report:
import-submit-utils.spec.ts

PipelineOverview.spec.tsx

pipeline-overview-utils.spec.ts

Test setup:

Install Openshift pipeilnes operator with preview channel to get 1.2 operator
Go to add from git flow
Use dotnet image https://github.com/redhat-developer/s2i-dotnetcore-ex.git and choose dotnet builder image
Add Pipeline option
click on create button

Browser conformance:

Chrome
Firefox
Safari
Edge

cc: @andrewballantyne @bgliwa01 @openshift/team-devconsole-ux

andrewballantyne · 2020-11-17T15:09:18Z

/assign

andrewballantyne · 2020-11-18T15:53:09Z

Ah, I missed AC in the Story, apologizes for that @karthikjeeyar - If you look at the design, we have a need to show a message in the sidebar.

🤔 To do this, perhaps we find a way to add some form of sessionStorage (NOT localStorage) value when it fails?

The read-only sessionStorage property accesses a session Storage object for the current origin. sessionStorage is similar to localStorage; the difference is that while data in localStorage doesn't expire, data in sessionStorage is cleared when the page session ends.

Ref: MDN Docs

Our goal here is to just inform the user immediately, not to persist it indefinitely.

frontend/packages/dev-console/src/components/import/__tests__/import-submit-utils.spec.ts

karthikjeeyar · 2020-11-19T11:53:21Z

@andrewballantyne I have updated the code to show the alert in the sidebar, updated screenshots and added more tests.

andrewballantyne · 2020-11-20T18:50:27Z

frontend/packages/dev-console/src/components/topology/redux/const.ts

@@ -1 +1,2 @@
 export const TOPOLOGY_DISPLAY_FILTERS_LOCAL_STORAGE_KEY = `bridge/topology-display-filters`;
+export const PIPELINE_RUN_AUTO_START_FAILED = `bridge/pipeline-run-auto-start-failed`;


Probably should not be Pipeline specific if we are putting it in this file 🤔

Perhaps we just move it and put it in frontend/packages/pipelines-plugin/src/components/pipelines/const.ts

andrewballantyne · 2020-11-20T18:51:56Z

...ckages/pipelines-plugin/src/components/pipelines/pipeline-overview/PipelineOverview.spec.tsx

@@ -51,4 +53,10 @@ describe('Pipeline sidebar overview', () => {
    const wrapper = shallow(<PipelineOverview {...props} />);
    expect(wrapper.find(TriggerLastRunButton)).toHaveLength(1);
  });
+
+  it('should show the pipeline not started Alert', () => {


Perhaps we should also write one more test to make sure it's not there.

andrewballantyne · 2020-11-20T18:52:28Z

...ckages/pipelines-plugin/src/components/pipelines/pipeline-overview/PipelineOverview.spec.tsx

+    expect(wrapper.find(PipelineOverviewAlert)).toHaveLength(1);
+  });


Can you "reset" it afterwards? That way if we want other tests, they are not impacted by this value.

andrewballantyne · 2020-11-20T19:01:27Z

...nd/packages/pipelines-plugin/src/components/pipelines/pipeline-overview/PipelineOverview.tsx

+  const [showAlert, setShowAlert] = React.useState(isPipelineNotStarted(name));
+
+  React.useEffect(() => {
+    setShowAlert(isPipelineNotStarted(name));
+  }, [name]);


I think we want to just live read it on each render... without the hooks. This way wouldn't need to worry so much about unsetting it when the user clicks the 'x'.

Suggested change

const [showAlert, setShowAlert] = React.useState(isPipelineNotStarted(name));

React.useEffect(() => {

setShowAlert(isPipelineNotStarted(name));

}, [name]);

const showAlert = isPipelineNotStarted(name);

removed UseEffect but still using local state as the re render is not quick enough to remove the Alert.

Ah I see... my bad. 🤔

andrewballantyne · 2020-11-20T19:01:57Z

...nd/packages/pipelines-plugin/src/components/pipelines/pipeline-overview/PipelineOverview.tsx

  return (
    <>
      <SidebarSectionHeading text={PipelineRunModel.labelPlural}>
+        {showAlert && (


Suggested change

{showAlert && (

{(showAlert && pipelineRuns.length === 0) && (

If they start up a Pipeline Run we don't want to show it.

andrewballantyne · 2020-11-20T19:10:54Z

...kages/pipelines-plugin/src/components/pipelines/pipeline-overview/pipeline-overview-utils.ts

+export const setPipelineNotStarted = (pipelineName: string): void => {
+  if (!pipelineName) return;
+  const pipelineList = getNotStartedPipelines();
+  if (pipelineList.length === 0 || !pipelineList.includes(pipelineName)) {
+    pipelineList.push(pipelineName);
+  }
+  sessionStorage.setItem(PIPELINE_RUN_AUTO_START_FAILED, JSON.stringify(pipelineList));
+};


We want to hold onto the namespace as well... if you just do name, I could switch between projects and get unexpected results.

Plus you probably want to only write it back out when you modify it... no need to write the same value you got back to the storage if it didn't change.

Suggested change

export const setPipelineNotStarted = (pipelineName: string): void => {

if (!pipelineName) return;

const pipelineList = getNotStartedPipelines();

if (pipelineList.length === 0 || !pipelineList.includes(pipelineName)) {

pipelineList.push(pipelineName);

}

sessionStorage.setItem(PIPELINE_RUN_AUTO_START_FAILED, JSON.stringify(pipelineList));

};

export const setPipelineNotStarted = (namespace: string, pipelineName: string): void => {

if (!pipelineName || !namespace) return;

const pipelineData = getNotStartedPipelines();

if (!pipelineData[namespace]) {

pipelineData[namespace] = [];

}

if (!pipelineData[namespace].includes(pipelineName)) {

pipelineData[namespace].push(pipelineName);

sessionStorage.setItem(PIPELINE_RUN_AUTO_START_FAILED, JSON.stringify(pipelineData));

}

};

andrewballantyne · 2020-11-20T19:11:26Z

...kages/pipelines-plugin/src/components/pipelines/pipeline-overview/pipeline-overview-utils.ts

+import { PIPELINE_RUN_AUTO_START_FAILED } from '@console/dev-console/src/components/topology/redux/const';
+
+export const getNotStartedPipelines = () => {
+  return JSON.parse(sessionStorage.getItem(PIPELINE_RUN_AUTO_START_FAILED) ?? '[]');


Based on my last comment... needs to be an object

andrewballantyne · 2020-11-20T19:12:35Z

...kages/pipelines-plugin/src/components/pipelines/pipeline-overview/pipeline-overview-utils.ts

+export const removePipelineNotStarted = (pipelineName: string): void => {
+  const newList = getNotStartedPipelines().filter((pName) => pName !== pipelineName);


Based on my last(last) comment... need namespace here.

@andrewballantyne Updated the code to store/remove a namespaced session object.

andrewballantyne · 2020-11-24T14:51:37Z

...nd/packages/pipelines-plugin/src/components/pipelines/pipeline-overview/PipelineOverview.tsx

@@ -31,9 +33,20 @@ const PipelinesOverview: React.FC<PipelinesOverviewProps> = ({
  const {
    metadata: { name, namespace },
  } = pipeline;
+  const [showAlert, setShowAlert] = React.useState(isPipelineNotStarted(name, namespace));


This is not reactive 😞

I'll need to do some investigation into why we can't just use it in the render.

Ah okay, I see what is happening here... yeah your original solution made more sense. I'm not sure if we can entirely base our data on the sessionStorage 🤔 (which was my original reason for my suggestion)

Yeah, I think we can't get a "single state" of storage here... which is unfortunately. Okay, reset it back to the useEffect hook and we should be good to go.

const [showAlert, setShowAlert] = React.useState(isPipelineNotStarted(name, namespace)); React.useEffect(() => { setShowAlert(isPipelineNotStarted(name, namespace)); }, [name, namespace]);

Sorry for this round-about solution... you had it right the first time.

andrewballantyne

Just need to rebase and reimplement the hook @karthikjeeyar - Get a LGTM from someone on the team or if I see the push I'll give it. Thanks!

karthikjeeyar · 2020-11-25T06:04:05Z

Rebased and added the hook back.

andrewballantyne

/lgtm

openshift-ci-robot · 2020-11-25T12:58:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, karthikjeeyar

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/dev-console/OWNERS~~ [andrewballantyne]
~~frontend/packages/pipelines-plugin/OWNERS~~ [andrewballantyne]

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 added the component/dev-console Related to dev-console label Nov 17, 2020

openshift-ci-robot requested review from jerolimov and nemesis09 November 17, 2020 14:04

openshift-ci-robot assigned andrewballantyne Nov 17, 2020

andrewballantyne suggested changes Nov 18, 2020

View reviewed changes

frontend/packages/dev-console/src/components/import/__tests__/import-submit-utils.spec.ts Show resolved Hide resolved

karthikjeeyar force-pushed the addflow-creation branch from 2e7edb7 to 687dcee Compare November 19, 2020 11:56

andrewballantyne suggested changes Nov 20, 2020

View reviewed changes

karthikjeeyar force-pushed the addflow-creation branch from 687dcee to 6067b97 Compare November 23, 2020 15:58

andrewballantyne reviewed Nov 24, 2020

View reviewed changes

andrewballantyne approved these changes Nov 24, 2020

View reviewed changes

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

add non blocking pipeline creation flow

27a3f37

karthikjeeyar force-pushed the addflow-creation branch from 6067b97 to 27a3f37 Compare November 25, 2020 06:03

andrewballantyne approved these changes Nov 25, 2020

View reviewed changes

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

openshift-merge-robot merged commit c8c5e16 into openshift:master Nov 25, 2020

karthikjeeyar mentioned this pull request Nov 27, 2020

Gherkin script for Pipelines - Improve Pipeline Creation Process- ODC… #7310

Merged

spadgett added this to the v4.7 milestone Nov 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add non blocking pipeline creation flow #7241

add non blocking pipeline creation flow #7241

karthikjeeyar commented Nov 17, 2020 •

edited

andrewballantyne commented Nov 17, 2020

andrewballantyne commented Nov 18, 2020

karthikjeeyar commented Nov 19, 2020

andrewballantyne Nov 20, 2020

andrewballantyne Nov 20, 2020

andrewballantyne Nov 20, 2020

andrewballantyne Nov 20, 2020

karthikjeeyar Nov 23, 2020

andrewballantyne Nov 24, 2020

andrewballantyne Nov 20, 2020

andrewballantyne Nov 20, 2020

andrewballantyne Nov 20, 2020

andrewballantyne Nov 20, 2020

karthikjeeyar Nov 23, 2020

andrewballantyne Nov 24, 2020

andrewballantyne Nov 24, 2020

andrewballantyne Nov 24, 2020

andrewballantyne left a comment

karthikjeeyar commented Nov 25, 2020

andrewballantyne left a comment

openshift-ci-robot commented Nov 25, 2020

		@@ -1 +1,2 @@
		export const TOPOLOGY_DISPLAY_FILTERS_LOCAL_STORAGE_KEY = `bridge/topology-display-filters`;
		export const PIPELINE_RUN_AUTO_START_FAILED = `bridge/pipeline-run-auto-start-failed`;

		expect(wrapper.find(PipelineOverviewAlert)).toHaveLength(1);
		});

	{showAlert && (
	{(showAlert && pipelineRuns.length === 0) && (

-export const setPipelineNotStarted = (pipelineName: string): void => {
-  if (!pipelineName) return;
-  const pipelineList = getNotStartedPipelines();
-  if (pipelineList.length === 0 || !pipelineList.includes(pipelineName)) {
-    pipelineList.push(pipelineName);
-  }
-  sessionStorage.setItem(PIPELINE_RUN_AUTO_START_FAILED, JSON.stringify(pipelineList));
-};
+export const setPipelineNotStarted = (namespace: string, pipelineName: string): void => {
+  if (!pipelineName || !namespace) return;
+  const pipelineData = getNotStartedPipelines();
+  if (!pipelineData[namespace]) {
+    pipelineData[namespace] = [];
+  }
+  if (!pipelineData[namespace].includes(pipelineName)) {
+    pipelineData[namespace].push(pipelineName);
+    sessionStorage.setItem(PIPELINE_RUN_AUTO_START_FAILED, JSON.stringify(pipelineData));
+  }
+};

		export const removePipelineNotStarted = (pipelineName: string): void => {
		const newList = getNotStartedPipelines().filter((pName) => pName !== pipelineName);

add non blocking pipeline creation flow #7241

add non blocking pipeline creation flow #7241

Conversation

karthikjeeyar commented Nov 17, 2020 • edited

andrewballantyne commented Nov 17, 2020

andrewballantyne commented Nov 18, 2020

karthikjeeyar commented Nov 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

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

andrewballantyne left a comment

Choose a reason for hiding this comment

karthikjeeyar commented Nov 25, 2020

andrewballantyne left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Nov 25, 2020

karthikjeeyar commented Nov 17, 2020 •

edited