Bug 1840264: fixes: delete application error #5573

sahil143 · 2020-05-26T17:55:23Z

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

Analysis / Root cause: Error is thrown for resources created through Deploy Image form because build configs are not present for the resource

Solution Description: If build config is not present, omit the delete buildconfig request

Screen shots / Gifs for design review:

Unit test coverage report: added

Test setup:

Browser conformance:

Chrome
Firefox
Safari
Edge

openshift-ci-robot · 2020-05-26T17:55:29Z

@sahil143: This pull request references Bugzilla bug 1840264, 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 1840264: fixes: delete application error

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.

vikram-raj

/lgtm

christianvogt · 2020-05-27T02:37:42Z

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

@@ -456,11 +458,11 @@ export const cleanUpWorkload = (
      deleteRequest(modelFor(resource.kind), resource);
      batchDeleteRequests(deleteModels, resource);
      deleteRequest(ImageStreamModel, resource); // delete imageStream
-      webhooksAvailable = true;
+      isBuildConfigPresent ? (webhooksAvailable = true) : (webhooksAvailable = false);


webhooksAvailable = isBuildConfigPresent ?

@sahil143 This may not be true always i.e if buildConfig is present then it'll have webhooks as even in UI we have option to select webhooks. So we may need to handle it

@sahil143 @invincibleJai so generic webhook is always going to be there since we are adding it always so here we can set webhooksAvailable based on buildConfig is present or not. Below at line no. 470 inside the if block we can check if some other webhook also exist using checkIfTriggerExists from edit-application-utils and if it exists then based on that add that hook into the webhooks array.

+1 for webhooksAvailable = isBuildConfigPresent;

divyanshiGupta · 2020-05-27T12:53:55Z

@sahil143 fix the unit tests.

sahil143 · 2020-05-27T13:00:23Z

@sahil143 fix the unit tests.
fixed!

divyanshiGupta · 2020-05-27T13:31:26Z

@sahil143 it seems the tests are still failing. PTAL.

sahil143 · 2020-05-27T13:40:52Z

/retest

invincibleJai · 2020-05-27T18:54:12Z

Verified with various flow git/container image works as expected for D/DC/KN

/lgtm

andrewballantyne · 2020-05-28T12:15:15Z

/assign @christianvogt
/retest

divyanshiGupta · 2020-05-28T12:50:32Z

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

+      if (t.generic) {
+        obj = {
+          ...resource,
+          metadata: {
+            name:
+              t.generic?.secretReference?.name ??
+              `${resource.metadata.name}-generic-webhook-secret`,
+            namespace: resource.metadata.namespace,
+          },
+        };
+      }
+      if (t[gitType] && !isKnativeResource) {
+        obj = {
+          ...resource,
+          metadata: {
+            name:
+              t[gitType]?.secretReference?.name ??
+              `${resource.metadata.name}-${gitType}-webhook-secret`,
+            namespace: resource.metadata.namespace,
+          },
+        };
+      }


Suggested change

if (t.generic) {

obj = {

...resource,

metadata: {

name:

t.generic?.secretReference?.name ??

`${resource.metadata.name}-generic-webhook-secret`,

namespace: resource.metadata.namespace,

},

};

}

if (t[gitType] && !isKnativeResource) {

obj = {

...resource,

metadata: {

name:

t[gitType]?.secretReference?.name ??

`${resource.metadata.name}-${gitType}-webhook-secret`,

namespace: resource.metadata.namespace,

},

};

}

const webhookType = t.generic ? 'generic' : gitType;

const webhookTypeObj = t.generic || (!isKnativeResource && t[gitType])

if (webhookTypeObj) {

obj = {

...resource,

metadata: {

name:

webhookTypeObj.secretReference?.name ??

`${resource.metadata.name}-${webhookType}-webhook-secret`,

namespace: resource.metadata.namespace,

},

};

}

@sahil143 something like this can be done to reduce the lines of code here because both the objects are similar except for the name. WDYT?

yeah, for these two cases make sense. updated

divyanshiGupta

/lgtm

andrewballantyne · 2020-05-28T18:42:03Z

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

+      }
+      return obj ? [...a, safeKill(SecretModel, obj)] : a;
+    }, []);
+    return [...requests, ...reqs];


Uncaught (in promise) TypeError: reqs is not iterable

If you don't get triggers it throws an exception. You need to guard against spreading undefined.

andrewballantyne · 2020-05-28T18:42:32Z

/lgtm cancel

do not send delete request for BCs if BC array is empty in resources add ansd fix unit test for application utils update delete webhooks logic update fix unit tests fix unit tests update code update delete webhook logic typeguard against undefiend for triggers

andrewballantyne

/lgtm

christianvogt · 2020-05-28T19:19:22Z

/approve

openshift-ci-robot · 2020-05-28T19:20:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, divyanshiGupta, invincibleJai, sahil143, vikram-raj

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-05-28T19:49:44Z

/retest

openshift-ci-robot · 2020-05-28T23:23:45Z

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

In response to this:

Bug 1840264: fixes: delete application error

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 the bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. label May 26, 2020

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/dev-console Related to dev-console labels May 26, 2020

openshift-ci-robot requested review from jeff-phillips-18 and vikram-raj May 26, 2020 17:55

vikram-raj reviewed May 26, 2020

View reviewed changes

openshift-ci-robot assigned vikram-raj May 26, 2020

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

christianvogt reviewed May 27, 2020

View reviewed changes

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

sahil143 force-pushed the delete-application-error branch from 04de48c to c5def17 Compare May 27, 2020 12:31

sahil143 force-pushed the delete-application-error branch from c5def17 to da45d21 Compare May 27, 2020 12:59

sahil143 force-pushed the delete-application-error branch from da45d21 to 84159d8 Compare May 27, 2020 13:33

openshift-ci-robot added the component/shared Related to console-shared label May 27, 2020

sahil143 force-pushed the delete-application-error branch from 84159d8 to 29f6f83 Compare May 27, 2020 18:25

openshift-ci-robot assigned invincibleJai May 27, 2020

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

openshift-ci-robot assigned christianvogt May 28, 2020

divyanshiGupta reviewed May 28, 2020

View reviewed changes

sahil143 force-pushed the delete-application-error branch from 29f6f83 to 70f09e6 Compare May 28, 2020 15:02

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

divyanshiGupta approved these changes May 28, 2020

View reviewed changes

openshift-ci-robot assigned divyanshiGupta May 28, 2020

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

andrewballantyne reviewed May 28, 2020

View reviewed changes

openshift-ci-robot assigned andrewballantyne May 28, 2020

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

sahil143 force-pushed the delete-application-error branch from 70f09e6 to 1dea830 Compare May 28, 2020 18:59

andrewballantyne approved these changes May 28, 2020

View reviewed changes

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

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

openshift-merge-robot merged commit 147577e into openshift:master May 28, 2020

spadgett added this to the v4.5 milestone Jun 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1840264: fixes: delete application error #5573

Bug 1840264: fixes: delete application error #5573

sahil143 commented May 26, 2020

openshift-ci-robot commented May 26, 2020

vikram-raj left a comment

christianvogt May 27, 2020

invincibleJai May 27, 2020

divyanshiGupta May 27, 2020 •

edited

jerolimov May 27, 2020

divyanshiGupta commented May 27, 2020

sahil143 commented May 27, 2020

divyanshiGupta commented May 27, 2020

sahil143 commented May 27, 2020

invincibleJai commented May 27, 2020

andrewballantyne commented May 28, 2020

divyanshiGupta May 28, 2020

sahil143 May 28, 2020

divyanshiGupta left a comment

andrewballantyne May 28, 2020

sahil143 May 28, 2020

andrewballantyne commented May 28, 2020

andrewballantyne left a comment

christianvogt commented May 28, 2020

openshift-ci-robot commented May 28, 2020

andrewballantyne commented May 28, 2020

openshift-ci-robot commented May 28, 2020

Bug 1840264: fixes: delete application error #5573

Bug 1840264: fixes: delete application error #5573

Conversation

sahil143 commented May 26, 2020

openshift-ci-robot commented May 26, 2020

vikram-raj left a comment

Choose a reason for hiding this comment

christianvogt May 27, 2020

Choose a reason for hiding this comment

invincibleJai May 27, 2020

Choose a reason for hiding this comment

divyanshiGupta May 27, 2020 • edited

Choose a reason for hiding this comment

jerolimov May 27, 2020

Choose a reason for hiding this comment

divyanshiGupta commented May 27, 2020

sahil143 commented May 27, 2020

divyanshiGupta commented May 27, 2020

sahil143 commented May 27, 2020

invincibleJai commented May 27, 2020

andrewballantyne commented May 28, 2020

divyanshiGupta May 28, 2020

Choose a reason for hiding this comment

sahil143 May 28, 2020

Choose a reason for hiding this comment

divyanshiGupta left a comment

Choose a reason for hiding this comment

andrewballantyne May 28, 2020

Choose a reason for hiding this comment

sahil143 May 28, 2020

Choose a reason for hiding this comment

andrewballantyne commented May 28, 2020

andrewballantyne left a comment

Choose a reason for hiding this comment

christianvogt commented May 28, 2020

openshift-ci-robot commented May 28, 2020

andrewballantyne commented May 28, 2020

openshift-ci-robot commented May 28, 2020

divyanshiGupta May 27, 2020 •

edited