feat(delete-group): Delete application group #2947

karthikjeeyar · 2019-10-10T11:08:03Z

This PR contains the custom actions and modals to delete an application from the topology page.

Added group actions available in the context menu
Added group actions in the sidebar menu

contains tests for all the supported workloads in topology.

 PASS  packages/dev-console/src/utils/__tests__/application-utils.spec.ts
  ApplicationUtils 
    ✓ Should delete all the specific models related to deployment config (25ms)
    ✓ Should delete all the specific models related to knative deployments (4ms)
    ✓ Should delete all the specific models related to daemonsets (4ms)
    ✓ Should delete all the specific models related to statefulsets (4ms)

Fixes: https://jira.coreos.com/browse/ODC-1795 , https://jira.coreos.com/browse/ODC-846

karthikjeeyar · 2019-10-10T11:10:53Z

frontend/packages/dev-console/src/components/import/git/GitSection.tsx

    const name = values.name || values.image.selected;
    values.name !== name && setFieldValue('name', name);
    !values.application.name && setFieldValue('application.name', `${name}-app`);
    setFieldValue('git.url', url);
    setFieldValue('git.dir', dir);
    setFieldValue('git.ref', ref);
+    setFieldValue('git.type', gitType);
+    setFieldTouched('git.url', true);


These above changes are added to address an existing bug in dev catalog flow (using Try sample option)

karthikjeeyar · 2019-10-10T12:19:00Z

/test e2e-aws-console-olm

karthikjeeyar · 2019-10-10T13:53:28Z

/test e2e-aws-console-olm

sahil143 · 2019-10-10T18:27:25Z

@karthikjeeyar I cant see the action dropdown in the sidebar. Am I missing something?

I can see the delete option while right-clicking on the group.

sahil143 · 2019-10-10T18:46:57Z

Question: What do we do about the leftover connects-to annotations?
If we have a node( with name nodejs) connected with one of the nodes(name nginx) in a group and later on that group is deleted. After deletion, we would have leftover connects-to annotation on nodejs pointing to nginx(which doesn't exist anymore). Should we also delete these annotations? If not, What if a new workload is created with the same name(nginx), as a side effect of leftover annotation we end up with a connector between newly created node(nginx) and old node(nodejs).

sahil143 · 2019-10-10T18:54:19Z

frontend/packages/dev-console/src/components/topology/actions-providers.ts

@@ -23,7 +24,9 @@ export class ActionProviders {
  public getEdgeActions = (edgeId: string): KebabOption[] => null;

  // eslint-disable-next-line @typescript-eslint/no-unused-vars, no-unused-vars


This eslint-disable-next-line can be removed now.

karthikjeeyar · 2019-10-11T04:18:20Z

Question: What do we do about the leftover connects-to annotations?
If we have a node( with name nodejs) connected with one of the nodes(name nginx) in a group and later on that group is deleted. After deletion, we would have leftover connects-to annotation on nodejs pointing to nginx(which doesn't exist anymore). Should we also delete these annotations? If not, What if a new workload is created with the same name(nginx), as a side effect of leftover annotation we end up with a connector between newly created node(nginx) and old node(nodejs).

I agree, we should remove/update the connects-to annotation in the other(connected) nodes as well, I will update the PR
cc: @christianvogt

invincibleJai

Overall looks good to me, will try it in a while

invincibleJai · 2019-10-11T05:15:39Z

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

+  resource: K8sResourceKind,
+  workload: TopologyDataObject,
+): Promise<K8sResourceKind[]> => {
+  // cleanup all the items for a workoad.


nit: can we remove comments as function name is intuitive.

invincibleJai · 2019-10-11T05:16:32Z

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

+
+  if (isKnativeResource) {
+    // delete knative resources
+    const knativeRoute = _.find(workload.resources.ksroutes, { kind: 'Route' });


it would be good to rely on knative services instead of routes if metadata.name is as expected

@invincibleJai Services are having generated names, looks like the only object to rely on to get the original name is ksroutes object.

@karthikjeeyar got the issue, knative services are not available in resources. what you saw would be k8s resources.

I am filling a PR on this branch to handle it.

karthikjeeyar#6

karthikjeeyar · 2019-10-11T05:55:58Z

@karthikjeeyar I cant see the action dropdown in the sidebar. Am I missing something?
I can see the delete option while right-clicking on the group

@Sahil Pushed the changes

debsmita1 · 2019-10-11T19:38:52Z

@karthikjeeyar A scrollbar on the right appears when I click on the Actions dropdown

karthikjeeyar · 2019-10-12T08:08:09Z

@karthikjeeyar A scrollbar on the right appears when I click on the Actions dropdown

@debsmita1 This is PR is just to add actions to the sidebar. since the content of the sidebar is empty we are seeing the scrollbar. This PR will be integrated with the work @invincibleJai has done related to the content of the sidebar on #2947. I have tested integrating both the PR's and the scrollbar isn't showing up.

invincibleJai · 2019-10-14T08:46:36Z

@karthikjeeyar have verified it locally changes looks good to me. Thanks!!

jeff-phillips-18 · 2019-10-14T13:52:20Z

frontend/packages/dev-console/src/components/modals/DeleteApplicationModal.tsx

+  const isValid = _.get(values, 'application.userInput') === initialApplication;
+  return (
+    <form onSubmit={handleSubmit} className="modal-content modal-content--no-inner-scroll">
+      <ModalTitle>Delete Application Group</ModalTitle>


Suggested change

<ModalTitle>Delete Application Group</ModalTitle>

<ModalTitle>Delete Application</ModalTitle>

jeff-phillips-18 · 2019-10-14T13:52:42Z

frontend/packages/dev-console/src/components/topology/actions/groupActions.ts

+  // so currently picking the first resource to do the rbac checks (might change in future)
+  const primaryResource = _.get(application.resources[0], ['resources', 'obj']);
+  return {
+    label: 'Delete Application Group',


Suggested change

label: 'Delete Application Group',

label: 'Delete Application',

jeff-phillips-18 · 2019-10-14T13:53:58Z

frontend/packages/dev-console/src/components/modals/DeleteApplicationModal.tsx

+              This action cannot be undone. All associated Deployments, Routes, Builds, Pipelines,
+              Storage/PVC&#39;s, secrets, configmaps will be deleted.


Suggested change

This action cannot be undone. All associated Deployments, Routes, Builds, Pipelines,

Storage/PVC's, secrets, configmaps will be deleted.

Storage/PVC's, secrets, and configmaps will be deleted.

jeff-phillips-18 · 2019-10-14T15:53:09Z

/lgtm

divyanshiGupta · 2019-10-14T18:59:12Z

frontend/packages/dev-console/src/utils/__tests__/application-utils.spec.ts

+
+import Spy = jasmine.Spy;
+
+const spyAndReturn = (spy: Spy) => (returnValue: any) =>


Can return value be typed?

spyAndReturn is a generic test helper method, which can take in any spy and will resolve any value that is passed to it. IMO, we cannot bound this to any particular type

divyanshiGupta · 2019-10-14T19:14:27Z

@karthikjeeyar I pulled this PR and tried it locally but I can't see the delete option on right click as well as not in the the sidebar.

karthikjeeyar · 2019-10-15T07:09:57Z

@karthikjeeyar I pulled this PR and tried it locally but I can't see the delete option on right click as well as not in the the sidebar.

@divyanshiGupta It could be your browser cache, please try hard reloading.

divyanshiGupta · 2019-10-15T09:05:05Z

@karthikjeeyar I pulled this PR and tried it locally but I can't see the delete option on right click as well as not in the the sidebar.

@divyanshiGupta It could be your browser cache, please try hard reloading.

Yes I did hard refresh a lot of times but it was not showing up but suddenly now it showed up :P It is working fine for me now.

divyanshiGupta · 2019-10-15T09:05:18Z

/lgtm

karthikjeeyar · 2019-10-15T13:31:05Z

/test e2e-aws-console-olm

karthikjeeyar · 2019-10-15T13:57:27Z

/test e2e-aws-console-olm

karthikjeeyar · 2019-10-15T16:21:47Z

/test e2e-aws-console-olm

divyanshiGupta · 2019-10-15T18:22:50Z

/lgtm

divyanshiGupta · 2019-10-15T18:26:25Z

/test e2e-aws-console-olm

rohitkrai03 · 2019-10-15T20:05:22Z

frontend/packages/dev-console/src/components/modals/DeleteApplicationModal.tsx

+  errorMessage: string;
+};
+
+const DeleteApplicationForm: React.FC<FormikProps<FormikValues> & DeleteApplicationModalProps> = ({


Nit: We should ideally have one component per file. Otherwise looks good to me.

rohitkrai03 · 2019-10-15T20:06:52Z

/lgtm

karthikjeeyar · 2019-10-16T05:50:37Z

/assign @christianvogt

christianvogt · 2019-10-16T13:24:58Z

/approve

openshift-ci-robot · 2019-10-16T13:25:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, divyanshiGupta, jeff-phillips-18, karthikjeeyar, 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

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/dev-console Related to dev-console labels Oct 10, 2019

openshift-ci-robot requested review from abhinandan13jan and joshuawilson October 10, 2019 11:08

karthikjeeyar commented Oct 10, 2019

View reviewed changes

sahil143 reviewed Oct 10, 2019

View reviewed changes

invincibleJai reviewed Oct 11, 2019

View reviewed changes

karthikjeeyar force-pushed the group-actions branch 2 times, most recently from 3ef076a to 41cf98b Compare October 11, 2019 13:44

karthikjeeyar force-pushed the group-actions branch from 41cf98b to 04f2f47 Compare October 14, 2019 06:38

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. component/knative Related to knative-plugin component/shared Related to console-shared and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 14, 2019

karthikjeeyar force-pushed the group-actions branch 2 times, most recently from 6c8c8ab to 0fe26d3 Compare October 14, 2019 10:59

jeff-phillips-18 reviewed Oct 14, 2019

View reviewed changes

karthikjeeyar force-pushed the group-actions branch from 0fe26d3 to 82b693e Compare October 14, 2019 14:13

jeff-phillips-18 approved these changes Oct 14, 2019

View reviewed changes

divyanshiGupta reviewed Oct 14, 2019

View reviewed changes

karthikjeeyar force-pushed the group-actions branch from 82b693e to e70aa3a Compare October 15, 2019 07:04

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2019

karthikjeeyar force-pushed the group-actions branch from e70aa3a to c43e251 Compare October 15, 2019 08:49

openshift-ci-robot assigned divyanshiGupta Oct 15, 2019

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

karthikjeeyar and others added 2 commits October 15, 2019 17:13

delete application group

63f4627

fetch knative service for knative and updates spec

5c787e1

karthikjeeyar force-pushed the group-actions branch from c43e251 to 5c787e1 Compare October 15, 2019 11:51

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2019

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

rohitkrai03 reviewed Oct 15, 2019

View reviewed changes

openshift-ci-robot assigned rohitkrai03 Oct 15, 2019

openshift-ci-robot assigned christianvogt Oct 16, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2019

openshift-merge-robot merged commit bb544eb into openshift:master Oct 16, 2019

spadgett added this to the v4.3 milestone Oct 16, 2019

		@@ -23,7 +24,9 @@ export class ActionProviders {
		public getEdgeActions = (edgeId: string): KebabOption[] => null;

		// eslint-disable-next-line @typescript-eslint/no-unused-vars, no-unused-vars

	<ModalTitle>Delete Application Group</ModalTitle>
	<ModalTitle>Delete Application</ModalTitle>

	label: 'Delete Application Group',
	label: 'Delete Application',

		This action cannot be undone. All associated Deployments, Routes, Builds, Pipelines,
		Storage/PVC's, secrets, configmaps will be deleted.

	This action cannot be undone. All associated Deployments, Routes, Builds, Pipelines,
	Storage/PVC's, secrets, configmaps will be deleted.
	Storage/PVC's, secrets, and configmaps will be deleted.


		import Spy = jasmine.Spy;

		const spyAndReturn = (spy: Spy) => (returnValue: any) =>

feat(delete-group): Delete application group #2947

feat(delete-group): Delete application group #2947

Conversation

karthikjeeyar commented Oct 10, 2019 • edited Loading

Choose a reason for hiding this comment

karthikjeeyar commented Oct 10, 2019

karthikjeeyar commented Oct 10, 2019

sahil143 commented Oct 10, 2019

sahil143 commented Oct 10, 2019

Choose a reason for hiding this comment

karthikjeeyar commented Oct 11, 2019 • edited Loading

invincibleJai left a comment • edited Loading

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 Oct 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikjeeyar commented Oct 11, 2019 • edited Loading

debsmita1 commented Oct 11, 2019

karthikjeeyar commented Oct 12, 2019

invincibleJai commented Oct 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-phillips-18 commented Oct 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divyanshiGupta commented Oct 14, 2019

karthikjeeyar commented Oct 15, 2019

divyanshiGupta commented Oct 15, 2019

divyanshiGupta commented Oct 15, 2019

karthikjeeyar commented Oct 15, 2019

karthikjeeyar commented Oct 15, 2019

karthikjeeyar commented Oct 15, 2019

divyanshiGupta commented Oct 15, 2019

divyanshiGupta commented Oct 15, 2019

Choose a reason for hiding this comment

rohitkrai03 commented Oct 15, 2019

karthikjeeyar commented Oct 16, 2019

christianvogt commented Oct 16, 2019

openshift-ci-robot commented Oct 16, 2019

karthikjeeyar commented Oct 10, 2019 •

edited

Loading

karthikjeeyar commented Oct 11, 2019 •

edited

Loading

invincibleJai left a comment •

edited

Loading

invincibleJai Oct 14, 2019 •

edited

Loading

karthikjeeyar commented Oct 11, 2019 •

edited

Loading