Bug 1776069: Uncaught exception in UI when getting webhooks links for buildconfigs #3733

jhadvig · 2019-12-09T18:18:34Z

Now we will fetch all the webhook secrets the loading the component.
Solves the clipboard issue in Firefox.

openshift-ci-robot · 2019-12-09T18:18:37Z

@jhadvig: This pull request references Bugzilla bug 1781300, which is invalid:

expected the bug to target the "4.4.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1781300: Uncaught exception in UI when getting webhooks links for buildconfigs

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.

spadgett

You should add some types for better type checking. In general, prefer the native Array.map and Array.find to the lodash versions (as long as you handle null values).

Looks like you'll have linter errors.

spadgett · 2019-12-09T18:21:19Z

frontend/public/components/utils/webhooks.tsx

+    ).then(response => {
+      setWebhookSecrets(response)
+    })
+  });


You need to specify dependencies or you'll request the secrets again on every render

spadgett · 2019-12-09T18:21:50Z

frontend/public/components/utils/webhooks.tsx

@@ -56,7 +56,26 @@ export const WebhookTriggers: React.FC<WebhookTriggersProps> = (props) => {
  });
  const tableColumnClasses = getTableColumnClasses(canGetSecret);
  const webhooks = _.filter(triggers, (trigger) => webhookTriggers.has(trigger.type));
-  if (_.isEmpty(webhooks)) {
+  const [webhookSecrets, setWebhookSecrets] = React.useState([])


Suggested change

const [webhookSecrets, setWebhookSecrets] = React.useState([])

const [webhookSecrets, setWebhookSecrets] = React.useState<K8sResourceKind>([])

spadgett · 2019-12-09T18:22:31Z

frontend/public/components/utils/webhooks.tsx

-  if (_.isEmpty(webhooks)) {
+  const [webhookSecrets, setWebhookSecrets] = React.useState([])
+
+  const webhookNames = _.map(webhooks, (webhook) => {


Suggested change

const webhookNames = _.map(webhooks, (webhook) => {

const webhookNames: string[] = _.map(webhooks, (webhook): string => {

spadgett · 2019-12-09T18:22:56Z

frontend/public/components/utils/webhooks.tsx

+  React.useEffect(() => {
+    Promise.all(
+      _.map(webhookNames, (webhookName) => {
+        return k8sGet(SecretModel, webhookName, namespace).then(res => {


Suggested change

return k8sGet(SecretModel, webhookName, namespace).then(res => {

return k8sGet(SecretModel, webhookName, namespace);

spadgett · 2019-12-09T18:24:16Z

frontend/public/components/utils/webhooks.tsx

-        errorModal({ error });
-      },
-    );
+    const webhookSecret = _.find(webhookSecrets, (secret) => { return secret.metadata.name === secretName})


const webhookSecret: K8sResourceKind = webhookSecrets.find((secret: K8sReosurceKind) => secret.metadata.name === secretName);

spadgett · 2019-12-09T18:25:41Z

frontend/public/components/utils/webhooks.tsx

+
+  React.useEffect(() => {
+    Promise.all(
+      _.map(webhookNames, (webhookName) => {


Suggested change

_.map(webhookNames, (webhookName) => {

_.map(webhookNames, (webhookName: string): Promise<K8sResourceKind> => {

openshift-ci-robot · 2019-12-09T19:26:47Z

@jhadvig: An error was encountered adding this pull request to the external tracker bugs for bug 1776069 on the Bugzilla server at https://bugzilla.redhat.com:

JSONRPC error 100500: Insecure dependency in parameter 3 of DBI::db=HASH(0x5620fa366c10)->do method call while running with -T switch at /loader/0x5620f51258a8/Bugzilla/Extension/ExternalBugs/Bug.pm line 327.
at /loader/0x5620f51258a8/Bugzilla/Extension/ExternalBugs/Bug.pm line 327.
Bugzilla::Extension::ExternalBugs::Bug::update_ext_info('Bugzilla::Extension::ExternalBugs::Bug=HASH(0x5620fa1a4138)', 1) called at /loader/0x5620f51258a8/Bugzilla/Extension/ExternalBugs/Bug.pm line 121
Bugzilla::Extension::ExternalBugs::Bug::create('Bugzilla::Extension::ExternalBugs::Bug', 'HASH(0x5620fa19b758)') called at /var/www/html/bugzilla/extensions/ExternalBugs/Extension.pm line 858
Bugzilla::Extension::ExternalBugs::bug_start_of_update('Bugzilla::Extension::ExternalBugs=HASH(0x5620f96d0730)', 'HASH(0x5620fa23edf0)') called at /var/www/html/bugzilla/Bugzilla/Hook.pm line 21
Bugzilla::Hook::process('bug_start_of_update', 'HASH(0x5620fa23edf0)') called at /var/www/html/bugzilla/Bugzilla/Bug.pm line 1170
Bugzilla::Bug::update('Bugzilla::Bug=HASH(0x5620f98b3ba0)') called at /loader/0x5620f51258a8/Bugzilla/Extension/ExternalBugs/WebService.pm line 80
Bugzilla::Extension::ExternalBugs::WebService::add_external_bug('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x5620fa21b1f8)') called at (eval 2708) line 1
eval ' $procedure->{code}->($self, @params)
;' called at /usr/share/perl5/vendor_perl/JSON/RPC/Legacy/Server.pm line 220
JSON::RPC::Legacy::Server::_handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x5620f9cab000)') called at /var/www/html/bugzilla/Bugzilla/WebService/Server/JSONRPC.pm line 295
Bugzilla::WebService::Server::JSONRPC::_handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x5620f9cab000)') called at /usr/share/perl5/vendor_perl/JSON/RPC/Legacy/Server.pm line 126
JSON::RPC::Legacy::Server::handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...') called at /var/www/html/bugzilla/Bugzilla/WebService/Server/JSONRPC.pm line 70
Bugzilla::WebService::Server::JSONRPC::handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...') called at /var/www/html/bugzilla/jsonrpc.cgi line 31
ModPerl::ROOT::Bugzilla::ModPerl::ResponseHandler::var_www_html_bugzilla_jsonrpc_2ecgi::handler('Apache2::RequestRec=SCALAR(0x5620fa138378)') called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 207
eval {...} called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 207
ModPerl::RegistryCooker::run('Bugzilla::ModPerl::ResponseHandler=HASH(0x5620f9f73168)') called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 173
ModPerl::RegistryCooker::default_handler('Bugzilla::ModPerl::ResponseHandler=HASH(0x5620f9f73168)') called at /usr/lib64/perl5/vendor_perl/ModPerl/Registry.pm line 32
ModPerl::Registry::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0x5620fa138378)') called at /var/www/html/bugzilla/mod_perl.pl line 139
Bugzilla::ModPerl::ResponseHandler::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0x5620fa138378)') called at (eval 2708) line 0
eval {...} called at (eval 2708) line 0

Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

Bug 1776069: Uncaught exception in UI when getting webhooks links for buildconfigs

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.

spadgett · 2019-12-09T19:27:33Z

Manually removing invalid bug since the bot is not working.

jhadvig · 2019-12-10T13:14:29Z

frontend/public/components/utils/webhooks.tsx

+    Promise.all(
+      webhookNames.map(
+        (webhookName: string): Promise<K8sResourceKind> => {
+          return k8sGet(SecretModel, webhookName, namespace).catch(() => {


Had to add the .catch() block since if there is an error retrieving the secret (eg. user manually edits the BC with a secret that does not exists...) we need to catch the error and return null , instead of uncaught error in promise

What happens when null is returned? It would be good to show a warning somewhere if we know the webhook secret doesn't exist and one of the benefits of fetching the secrets ahead. Although we'd need to differentiate between 403 errors and other errors.

jhadvig · 2019-12-10T13:14:55Z

@spadgett comments addressed. PTAL

spadgett · 2019-12-10T14:19:42Z

frontend/public/components/utils/webhooks.tsx

-  const webhooks = _.filter(triggers, (trigger) => webhookTriggers.has(trigger.type));
-  if (_.isEmpty(webhooks)) {
+  const [webhookSecrets, setWebhookSecrets] = React.useState<K8sResourceKind[]>([]);
+  const [isLoaded, setIsLoaded] = React.useState<boolean>(false);


This is fine, although boolean in inferred from the default value in this case, so not strictly necessary.

spadgett · 2019-12-10T14:22:05Z

frontend/public/components/utils/webhooks.tsx

+    Promise.all(
+      webhookNames.map(
+        (webhookName: string): Promise<K8sResourceKind> => {
+          return k8sGet(SecretModel, webhookName, namespace).catch(() => {


What happens when null is returned? It would be good to show a warning somewhere if we know the webhook secret doesn't exist and one of the benefits of fetching the secrets ahead. Although we'd need to differentiate between 403 errors and other errors.

spadgett · 2019-12-10T15:26:31Z

Bug 1776069 correctly targets 4.4. Manually adding the valid bug label since the bot is broken.

spadgett · 2019-12-10T15:27:08Z

https://bugzilla.redhat.com/show_bug.cgi?id=1776069

jhadvig · 2019-12-10T15:58:32Z

/retest

eparis · 2019-12-11T01:45:51Z

/bugzilla refresh

openshift-ci-robot · 2019-12-11T01:45:55Z

@eparis: This pull request references Bugzilla bug 1776069, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

/bugzilla refresh

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.

jhadvig · 2019-12-11T16:46:07Z

@spadget I've updated the PR with additional commit thats handling with the secret request errors. If any error occurs will display it in the ExpandableAlerts component.
screens:

If the use will try to copy the webhook error modal will be shown with a generic msg(or maybe just disable the link button?):

Although we'd need to differentiate between 403 errors and other errors.

Here we can just check the canGetSecret value in the React.useEffect block, since otherwise we are not loading the secrets at all...

spadgett

Thanks @jhadvig. The nice thing about this approach is we can now show a warning when a webhook secret doesn't exist 👍

spadgett · 2019-12-11T19:20:36Z

frontend/public/components/utils/webhooks.tsx

+              (error) => {
+                errors = _.concat(errors, error.message);


Suggested change

(error) => {

errors = _.concat(errors, error.message);

({message = `Could not load secret ${webhookName}.`}) => {

errors = [...errors, message];

Would rather keep the error message, since it could differ

Would rather keep the error message, since it could differ

We are keeping the error message, just using es6 syntax instead of _.concat

spadgett · 2019-12-11T19:22:13Z

frontend/public/components/utils/webhooks.tsx

+
+  React.useEffect(() => {
+    if (canGetSecret) {
+      let errors = [];


Suggested change

let errors = [];

let errors: string[] = [];

spadgett · 2019-12-11T19:23:29Z

frontend/public/components/utils/webhooks.tsx

+  const [secretErrors, setSecretErrors] = React.useState<string[]>([]);
+  const [isLoaded, setIsLoaded] = React.useState(false);
+
+  const webhooks: WebhookTrigger[] = _.filter(triggers, (trigger) =>


nit: you can use es6 syntactic sugar

Suggested change

const webhooks: WebhookTrigger[] = _.filter(triggers, (trigger) =>

const webhooks: WebhookTrigger[] = _.filter(triggers, ({type}) =>

webhookTriggers.has(type),

);

spadgett · 2019-12-11T19:24:25Z

frontend/public/components/utils/webhooks.tsx

-  const webhooks = _.filter(triggers, (trigger) => webhookTriggers.has(trigger.type));
+  const [webhookSecrets, setWebhookSecrets] = React.useState<K8sResourceKind[]>([]);
+  const [secretErrors, setSecretErrors] = React.useState<string[]>([]);
+  const [isLoaded, setIsLoaded] = React.useState(false);


Suggested change

const [isLoaded, setIsLoaded] = React.useState(false);

const [isLoaded, setLoaded] = React.useState(false);

spadgett · 2019-12-11T19:26:40Z

frontend/public/components/utils/webhooks.tsx

    );
+    if (!webhookSecret) {
+      errorModal({
+        error: `Could not get ${secretName} secret`,


Instead of showing an error, I would just remove the copy link when the secret is missing. We know it won't work.

We might want to also show the secret name as plain text instead of a link if an error occurred fetching it. Most likely it doesn't exist and will be a broken link. Checking for a 404 status code would tell us for sure.

spadgett · 2019-12-11T19:34:32Z

frontend/public/components/utils/webhooks.tsx

+  );
+
+  React.useEffect(() => {
+    if (canGetSecret) {


I'd suggest an early return here.

if (!canGetSecret) { return; }

jhadvig · 2019-12-12T11:48:05Z

@spadgett PR updated.

screen, with missing secret:

Regarding the wording on the error I would rather display the error message to the user.

PTAL

jhadvig · 2019-12-12T16:49:38Z

/retest

spadgett · 2019-12-16T16:41:35Z

frontend/public/components/utils/webhooks.tsx

+  const webhookNames: string[] = webhooks.map(
+    (webhook: WebhookTrigger): string => {
+      const triggerProperty = getTriggerProperty(webhook);
+      return _.get(webhook, [triggerProperty, 'secretReference', 'name']);


Won't this try to fetch secrets for webhooks that don't have a secret reference? You should probably use reduce to skip webhooks without a secret reference. Consider _.uniq in case multiple webhooks reference the same secret.

I'd also call the var secretNames to make it more clear it's the name of the secret itself.

const secretNames: Set<string> = _.uniq(webhooks.reduce((acc: string[], webhook: WebhookTrigger): string[] => { const triggerProperty = getTriggerProperty(webhook); const name = _.get(webhook, [triggerProperty, 'secretReference', 'name']); return name ? [...acc, name] : acc; }, []));

spadgett · 2019-12-16T19:22:58Z

frontend/public/components/utils/webhooks.tsx

+        <ExpandableAlert
+          variant={AlertVariant.warning}
+          alerts={_.map(secretErrors, (error, i) => (
+            <React.Fragment key={i}>{error}</React.Fragment>


Should this be a div so each alert is on its own line?

Suggested change

<React.Fragment key={i}>{error}</React.Fragment>

<div className="co-pre-line" key={i}>{error}</div>

jhadvig · 2019-12-16T22:23:56Z

@spadgett PR updated.
Screen:

… buildconfigs

spadgett

/approve
/lgtm

spadgett · 2019-12-17T14:06:41Z

/retest

openshift-ci-robot · 2019-12-17T14:06:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhadvig, spadgett

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~~ [jhadvig,spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2019-12-17T14:54:30Z

/retest

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

openshift-bot · 2019-12-17T16:51:26Z

/retest

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

openshift-ci-robot · 2019-12-17T18:51:10Z

@jhadvig: All pull requests linked via external trackers have merged. Bugzilla bug 1776069 has been moved to the MODIFIED state.

In response to this:

Bug 1776069: Uncaught exception in UI when getting webhooks links for buildconfigs

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 assigned spadgett Dec 9, 2019

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Dec 9, 2019

openshift-ci-robot added component/core Related to console core functionality size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 9, 2019

openshift-ci-robot requested review from benjaminapetersen and TheRealJon December 9, 2019 18:18

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

spadgett reviewed Dec 9, 2019

View reviewed changes

spadgett changed the title ~~Bug 1781300: Uncaught exception in UI when getting webhooks links for buildconfigs~~ Bug 1776069: Uncaught exception in UI when getting webhooks links for buildconfigs Dec 9, 2019

spadgett removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Dec 9, 2019

jhadvig force-pushed the bz1781300 branch from 588d9bb to 5d03360 Compare December 10, 2019 13:06

jhadvig commented Dec 10, 2019

View reviewed changes

spadgett reviewed Dec 10, 2019

View reviewed changes

spadgett added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Dec 10, 2019

spadgett added this to the v4.4 milestone Dec 10, 2019

spadgett reviewed Dec 11, 2019

View reviewed changes

jhadvig force-pushed the bz1781300 branch from 59fefaf to e1deec3 Compare December 12, 2019 11:44

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 12, 2019

jhadvig force-pushed the bz1781300 branch from e1deec3 to db6f502 Compare December 12, 2019 11:45

spadgett reviewed Dec 16, 2019

View reviewed changes

jhadvig force-pushed the bz1781300 branch from db6f502 to 8dadc8b Compare December 16, 2019 19:05

spadgett reviewed Dec 16, 2019

View reviewed changes

jhadvig force-pushed the bz1781300 branch from 8dadc8b to 72450dd Compare December 16, 2019 22:22

Bug 1781300: Uncaught exception in UI when getting webhooks links for…

59e4063

… buildconfigs

jhadvig force-pushed the bz1781300 branch from 72450dd to 59e4063 Compare December 17, 2019 11:05

spadgett approved these changes Dec 17, 2019

View reviewed changes

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

openshift-merge-robot merged commit 22e97eb into openshift:master Dec 17, 2019

	const [webhookSecrets, setWebhookSecrets] = React.useState([])
	const [webhookSecrets, setWebhookSecrets] = React.useState<K8sResourceKind>([])

	const webhookNames = _.map(webhooks, (webhook) => {
	const webhookNames: string[] = _.map(webhooks, (webhook): string => {

	return k8sGet(SecretModel, webhookName, namespace).then(res => {
	return k8sGet(SecretModel, webhookName, namespace);

	_.map(webhookNames, (webhookName) => {
	_.map(webhookNames, (webhookName: string): Promise<K8sResourceKind> => {

-  const webhooks: WebhookTrigger[] = _.filter(triggers, (trigger) =>
+  const webhooks: WebhookTrigger[] = _.filter(triggers, ({type}) =>
+    webhookTriggers.has(type),
+  );

	const [isLoaded, setIsLoaded] = React.useState(false);
	const [isLoaded, setLoaded] = React.useState(false);

	<React.Fragment key={i}>{error}</React.Fragment>
	<div className="co-pre-line" key={i}>{error}</div>

Bug 1776069: Uncaught exception in UI when getting webhooks links for buildconfigs #3733

Bug 1776069: Uncaught exception in UI when getting webhooks links for buildconfigs #3733

Conversation

jhadvig commented Dec 9, 2019

openshift-ci-robot commented Dec 9, 2019

spadgett left a comment • 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

openshift-ci-robot commented Dec 9, 2019

spadgett commented Dec 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhadvig commented Dec 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Dec 10, 2019

spadgett commented Dec 10, 2019

jhadvig commented Dec 10, 2019

eparis commented Dec 11, 2019

openshift-ci-robot commented Dec 11, 2019

jhadvig commented Dec 11, 2019

spadgett 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

jhadvig commented Dec 12, 2019

jhadvig commented Dec 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhadvig commented Dec 16, 2019

spadgett left a comment

Choose a reason for hiding this comment

spadgett commented Dec 17, 2019

openshift-ci-robot commented Dec 17, 2019

openshift-bot commented Dec 17, 2019

openshift-bot commented Dec 17, 2019

openshift-ci-robot commented Dec 17, 2019

spadgett left a comment •

edited