Add edit application functionality from topology #3788

divyanshiGupta · 2019-12-16T12:42:32Z

Jira stories: https://jira.coreos.com/browse/ODC-2346, https://jira.coreos.com/browse/ODC-2349

In this PR I have added the edit application scenario for the applications created using import from git/dockerfile/catalog flow. Knative service editing scenarios are also handled.

ToDo -

Populate advanced limits section

Note : I will be doing the following things in remaining story(from: deployImage) of edit-application epic in the next sprint:

I will be adding the unit tests all at once
Aligning UI with final UXD if present

frontend/packages/dev-console/src/components/edit-application/edit-application-utils.ts

sahil143 · 2019-12-16T14:42:13Z

@divyanshiGupta If I create a new source secret from the Edit Application modal then the Source Secret modal opens up but the Edit Application modal closes in the background.

P.S.: Not sure if we should be opening one modal over another modal.

sahil143 · 2019-12-16T14:49:02Z

@divyanshiGupta While editing the application if I change the application grouping name by creating a new one:

It doesn't change the application grouping name
It rollouts a new version of the resource

divyanshiGupta · 2019-12-16T15:03:14Z

@divyanshiGupta If I create a new source secret from the Edit Application modal then the Source Secret modal opens up but the Edit Application modal closes in the background.

P.S.: Not sure if we should be opening one modal over another modal.

@serenamarie125 @parvathyvr how should we handle this edge case?

divyanshiGupta · 2019-12-17T13:33:59Z

@divyanshiGupta While editing the application if I change the application grouping name by creating a new one:

It doesn't change the application grouping name

It rollouts a new version of the resource

Fixed.

frontend/packages/dev-console/src/components/edit-application/edit-application-utils.ts

frontend/packages/dev-console/src/components/edit-application/EditApplicationForm.tsx

frontend/packages/dev-console/src/components/edit-application/edit-application-utils.ts

divyanshiGupta · 2019-12-18T11:02:25Z

/test e2e-gcp-console
/test analyze

abhi-kn · 2019-12-19T08:06:45Z

frontend/packages/dev-console/src/components/edit-application/EditApplication.tsx

+      name: namespace,
+    },
+    git: EditApplicationUtils.getGitData(_.get(resources, 'buildConfig.data')),
+    docker: {


Any specific reason to not have util to get docker data in edit-application-utils?

So I created utils for the sections that were most complex to populate with data and have a single source of data. Docker section needs only the port information and docker file path and we get that info from two different resources so didnt created a specific util for this.

abhi-kn · 2019-12-19T08:07:21Z

frontend/packages/dev-console/src/components/edit-application/EditApplication.tsx

+        10,
+      ),
+    },
+    image: {


Any specific reason to not have util to get image data in edit-application-utils?

Same reason as above.

abhi-kn · 2019-12-19T10:23:29Z

frontend/packages/dev-console/src/components/edit-application/EditApplicationForm.tsx

+  <form className="modal-content" onSubmit={handleSubmit}>
+    <ModalTitle>Edit Application</ModalTitle>
+    <ModalBody>
+      {!_.isEmpty(values.build.strategy) && <GitSection />}


Is it better to check for Source strategy here?

We show GitSection for both strategies Source and Docker and hence I am just checking if there is a strategy show GitSection and if empty do not show it.

serenamarie125 · 2019-12-19T12:50:27Z

@divyanshiGupta If I create a new source secret from the Edit Application modal then the Source Secret modal opens up but the Edit Application modal closes in the background.

P.S.: Not sure if we should be opening one modal over another modal.

couple of points:

there was a change that Edit should be a full page form
modals from modals should work regardless, and work in other areas of OpenShift today

divyanshiGupta · 2019-12-19T13:09:17Z

@divyanshiGupta If I create a new source secret from the Edit Application modal then the Source Secret modal opens up but the Edit Application modal closes in the background.

P.S.: Not sure if we should be opening one modal over another modal.

couple of points:

there was a change that Edit should be a full page form

modals from modals should work regardless, and work in other areas of OpenShift today

@serenamarie125 I will be aligning with the final UXD in the deploy image story this sprint since I see that the doc has some unresolved comments and is not closed yet.

frontend/packages/dev-console/src/components/edit-application/EditApplication.tsx

frontend/packages/dev-console/src/components/edit-application/edit-application-types.ts

frontend/packages/dev-console/src/components/edit-application/edit-application-utils.ts

frontend/packages/dev-console/src/components/edit-application/edit-application-types.ts

frontend/packages/dev-console/src/components/import/app/AppSection.tsx

frontend/packages/dev-console/src/utils/shared-submit-utils.ts

frontend/packages/dev-console/src/components/modals/index.ts

christianvogt · 2019-12-19T19:27:58Z

That modal in the gif needs to be wider.

christianvogt · 2019-12-19T19:33:23Z

There's a few spacing issues now between form sections. for example:

divyanshiGupta · 2019-12-19T19:33:46Z

That modal in the gif needs to be wider.

I know but this is the modal we use across console and I didnt wanted to make any css changes since I knew things are going to change and now as we know its no more a modal but a page again. Just wanted to get the first pass done and deal with aligning with UX. Will align with final UXD in deploy image story 🙈

christianvogt · 2019-12-19T19:41:33Z

Oh we circled back once again to a page? hah. I can't keep up!

divyanshiGupta · 2019-12-19T19:46:11Z

Oh we circled back once again to a page? hah. I can't keep up!

Yes, I was also not aware of that change and then I looked at the doc and I was like where did the modal go. 😪

sahil143 · 2019-12-24T12:46:04Z

@divyanshiGupta Sometimes the resource object modifies behind the scene and we end up with this error.

We have seen this while editing YAML for a resource. For that, there is an option to reload the YAML. Maybe something similar for this as well?

cc: @serenamarie125

sahil143 · 2019-12-24T13:29:04Z

@divyanshiGupta Whenever a label is edited and then saved it triggers a new rollout and also it takes really long to open the modal.

divyanshiGupta · 2019-12-26T06:12:02Z

@divyanshiGupta Sometimes the resource object modifies behind the scene and we end up with this error.

We have seen this while editing YAML for a resource. For that, there is an option to reload the YAML. Maybe something similar for this as well?

cc: @serenamarie125

@sahil143 sure we can look at adding something similar. @serenamarie125 should we add a similar page reload button or there will be a different UX for this case?

rohitkrai03 · 2019-12-26T19:38:37Z

@divyanshiGupta Sometimes the resource object modifies behind the scene and we end up with this error.

We have seen this while editing YAML for a resource. For that, there is an option to reload the YAML. Maybe something similar for this as well?

cc: @serenamarie125

@sahil143 sure we can look at adding something similar. @serenamarie125 should we add a similar page reload button or there will be a different UX for this case?

@divyanshiGupta I think this problem will be solved if you use the deployment resources fetched by firehose instead of sending it from the modal. That way it will automatically get updated. I think you are working on the deploy image edit story where you're going to convert this to a full page from modal, for that you'll have to fetch the resource using firehose anyway.

rohitkrai03

/approve

rohitkrai03 · 2019-12-27T14:05:34Z

/lgtm

rohitkrai03 · 2019-12-27T14:05:52Z

/assign @christianvogt

divyanshiGupta · 2020-01-02T07:49:24Z

/test analyze
/test e2e-gcp-console

andrewballantyne

Please see my 👀 flagged comments, there may not be any changes needed but some questions to be answered.

andrewballantyne · 2019-12-31T20:24:21Z

frontend/packages/dev-console/src/actions/modify-application.ts

@@ -20,3 +20,17 @@ export const ModifyApplication = (kind, obj): KebabOption => {
    },
  };
 };
+
+export const EditApplication = (model, obj): KebabOption => {


Suggested change

export const EditApplication = (model, obj): KebabOption => {

export const EditApplication = (model: K8sKind, obj: K8sResourceKind): KebabOption => {

I'd appreciate it if you updated the ModifyApplication option as well. We should strive to type everything.

andrewballantyne · 2019-12-31T20:29:48Z

frontend/packages/dev-console/src/components/edit-application/EditApplication.tsx

+      editAppResource,
+    )
+      .then(() => {
+        actions.setSubmitting(false);


Disable submitting just before you submit? Any reason why?

If you see the submitting is getting disabled after the promise is resolved, i.e when the form is already submitted.

We discussed, I misread the code. No issue here.

andrewballantyne · 2019-12-31T20:30:21Z

frontend/packages/dev-console/src/components/edit-application/EditApplication.tsx

+    )
+      .then(() => {
+        actions.setSubmitting(false);
+        actions.setStatus({ error: '' });


Is error different from submitError in the catch block? If so, did we want to clear both?

andrewballantyne · 2019-12-31T20:39:13Z

frontend/packages/dev-console/src/components/edit-application/EditApplicationForm.tsx

+  status,
+  isSubmitting,
+}) => (
+  <form className="modal-content" onSubmit={handleSubmit}>


Hmmm... Would we not want to use import { Form } from '@patternfly/react-core';? HTML forms refresh the page on submit (and this needs to be suppressed usually - perhaps Formik handles this for us).

Using Form from pf as a modal wrapper introduces css issues and that is why form is being used here and at other places also where modal is being rendered. Anyways according to new UX we need to show a page and not a modal and I am aligning with new UX in my current story so form will be replaced by Form.

andrewballantyne · 2020-01-02T13:13:06Z

frontend/packages/dev-console/src/components/import/advanced/ResourceLimitSection.tsx

@@ -12,7 +15,7 @@ const ResourceLimitSection: React.FC = () => {
        label="Request"
        unitName="limits.cpu.requestUnit"
        unitOptions={CPUUnits}
-        defaultUnitSize="m"
+        defaultUnitSize={`${cpuLimits.value.requestUnit}`}


Oh boy this makes me uncomfortable. default based props are not supposed to change. I followed it down into the underlying component and it feels wrong there too... so I am unsure what to do here. I don't think this change is needed for it to work but I haven't fully tested.

@divyanshiGupta if this change was to fix the storing of the correct information then please keep it. If it is not a needed change, I think we shouldn't do it... but overall I don't think it's super important one way or another. It just feels wrong using a default prop as the live update value.

I made this change to show the unit size that was selected when the application was created as the default. I can create a separate prop in limits section for the default value which will be only initialized once and will not update with selecting another unit from the dropdown.

andrewballantyne · 2020-01-02T13:18:49Z

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

@@ -153,15 +156,18 @@ export const createBuildConfig = (
  };

  const buildConfig = {


Suggestion (for future changes):

_.merge({}, (originalBuildConfig || {}), newData)

andrewballantyne · 2020-01-02T13:22:24Z

frontend/packages/dev-console/src/components/import/import-types.ts

@@ -124,6 +125,7 @@ export interface DockerData {
 }

 export interface RouteData {
+  show?: boolean;


Isn't this always there? Or does RouteData get used by an interface that doesn't have this functionality yet?

So we need to show the create route option only when the route was not created when the application was created. If a route already exists we need not show the create route option. Now according to new UX we need to disable the option and not hide it. Aligning with new UX in my current story.

andrewballantyne

/approve

/assign @christianvogt

andrewballantyne

/lgtm
/approve

Christian will have to give the actual approval. But for all intensive purposes (until the e2e tests are fixed) this is reviewed.

andrewballantyne · 2020-01-02T16:21:38Z

/hold

andrewballantyne · 2020-01-02T19:44:08Z

/kind feature

christianvogt · 2020-01-02T20:03:43Z

/approve

openshift-ci-robot · 2020-01-02T20:04:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, divyanshiGupta, rohitkrai03

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

andrewballantyne · 2020-01-02T23:40:18Z

/hold cancel
/retest
#3847 has fixed the e2e tests

openshift-bot · 2020-01-02T23:55:12Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-03T00:06:39Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-03T01:24:46Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-03T01:37:52Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 16, 2019

openshift-ci-robot requested review from abhi-kn and andrewballantyne December 16, 2019 12:43

openshift-ci-robot added component/dev-console Related to dev-console component/knative Related to knative-plugin component/shared Related to console-shared labels Dec 16, 2019

sahil143 reviewed Dec 16, 2019

View reviewed changes

divyanshiGupta changed the title ~~Add edit application functionality from topology~~ [WIP]Add edit application functionality from topology Dec 16, 2019

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 16, 2019

divyanshiGupta changed the title ~~[WIP]Add edit application functionality from topology~~ Add edit application functionality from topology Dec 17, 2019

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 Dec 17, 2019

debsmita1 reviewed Dec 18, 2019

View reviewed changes

frontend/packages/dev-console/src/components/edit-application/edit-application-utils.ts Show resolved Hide resolved

debsmita1 reviewed Dec 18, 2019

View reviewed changes

abhi-kn reviewed Dec 19, 2019

View reviewed changes

christianvogt reviewed Dec 19, 2019

View reviewed changes

rohitkrai03 approved these changes Dec 27, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 27, 2019

openshift-ci-robot assigned christianvogt Dec 27, 2019

andrewballantyne reviewed Jan 2, 2020

View reviewed changes

andrewballantyne approved these changes Jan 2, 2020

View reviewed changes

Add edit application functionality from topology

6aca974

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

andrewballantyne approved these changes Jan 2, 2020

View reviewed changes

openshift-ci-robot assigned andrewballantyne Jan 2, 2020

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

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 2, 2020

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

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

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 2, 2020

openshift-merge-robot merged commit cbe705a into openshift:master Jan 3, 2020

spadgett added this to the v4.4 milestone Jan 3, 2020

divyanshiGupta mentioned this pull request Jan 28, 2020

Bug 1798357: Bump formik #4087

Merged

	export const EditApplication = (model, obj): KebabOption => {
	export const EditApplication = (model: K8sKind, obj: K8sResourceKind): KebabOption => {

		@@ -153,15 +156,18 @@ export const createBuildConfig = (
		};

		const buildConfig = {

Add edit application functionality from topology #3788

Add edit application functionality from topology #3788

Conversation

divyanshiGupta commented Dec 16, 2019 • edited

sahil143 commented Dec 16, 2019 • edited

sahil143 commented Dec 16, 2019

divyanshiGupta commented Dec 16, 2019

divyanshiGupta commented Dec 17, 2019

divyanshiGupta commented Dec 18, 2019

Choose a reason for hiding this comment

divyanshiGupta Dec 19, 2019 • 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

serenamarie125 commented Dec 19, 2019

divyanshiGupta commented Dec 19, 2019 • edited

christianvogt commented Dec 19, 2019

christianvogt commented Dec 19, 2019

divyanshiGupta commented Dec 19, 2019

christianvogt commented Dec 19, 2019

divyanshiGupta commented Dec 19, 2019

sahil143 commented Dec 24, 2019

sahil143 commented Dec 24, 2019

divyanshiGupta commented Dec 26, 2019

rohitkrai03 commented Dec 26, 2019

rohitkrai03 left a comment

Choose a reason for hiding this comment

rohitkrai03 commented Dec 27, 2019

rohitkrai03 commented Dec 27, 2019

divyanshiGupta commented Jan 2, 2020

andrewballantyne left a 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

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne commented Jan 2, 2020

andrewballantyne commented Jan 2, 2020

christianvogt commented Jan 2, 2020

openshift-ci-robot commented Jan 2, 2020

andrewballantyne commented Jan 2, 2020

openshift-bot commented Jan 2, 2020

openshift-bot commented Jan 3, 2020

openshift-bot commented Jan 3, 2020

openshift-bot commented Jan 3, 2020

divyanshiGupta commented Dec 16, 2019 •

edited

sahil143 commented Dec 16, 2019 •

edited

divyanshiGupta Dec 19, 2019 •

edited

divyanshiGupta commented Dec 19, 2019 •

edited