Bug 1652645 - Webhook urls in configuration page are invalid with undefined included #3097
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
app/views/browse/build-config.html
Outdated
| </dt> | ||
| <dd> | ||
| <copy-to-clipboard clipboard-text="buildConfig.metadata.name | webhookURL : trigger.type : trigger.generic : project.metadata.name : webhookSecrets"></copy-to-clipboard> | ||
| <div ng-if="trigger.generic | isWebhookAvailable : webhookSecrets"> |
There was a problem hiding this comment.
Isn't this potentially a problem for all webhook types, not only generic webhooks?
There was a problem hiding this comment.
yeah its definitely problem of all the webhook triggers... this is just a WIP
app/scripts/filters/resources.js
Outdated
| return null; | ||
| }; | ||
| }) | ||
| .filter('isWebhookAvailable', function(SecretsService) { |
There was a problem hiding this comment.
I'd suggest having the webhookURL filter return an empty string instead of creating a separate filter.
openshift-ci-robot
added
size/M
jhadvig
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
@spadgett I've updated the PR and removed the WIP flag. PTAL |
| if (canIFilter(secretsVersion, 'list')) { | ||
| secret = SecretsService.getWebhookSecretValue(secret, webhookSecrets); | ||
| if (!secret) { | ||
| return ''; |
There was a problem hiding this comment.
We could return secret, but was rather explicit with the return value here.
spadgett
left a comment
spadgett
left a comment
There was a problem hiding this comment.
One small comment, but I don't think it's an issue in practice. Tagging for merge. Thanks 👍
/lgtm
| if (_.get(secret, 'secretReference.name') && webhookSecrets) { | ||
| var matchingSecret = _.find(webhookSecrets, {metadata:{name: secret.secretReference.name}}); | ||
| return decodeSecretData(matchingSecret.data).WebHookSecretKey; | ||
| return _.has(matchingSecret, 'data') ? decodeSecretData(matchingSecret.data).WebHookSecretKey : ''; |
There was a problem hiding this comment.
Prefer _.get so we never try to decode null values. (_.has only checks for undefined.)
| return _.has(matchingSecret, 'data') ? decodeSecretData(matchingSecret.data).WebHookSecretKey : ''; | |
| return _.get(matchingSecret, 'data') ? decodeSecretData(matchingSecret.data).WebHookSecretKey : ''; |
|
We should fix in 3.10 and 3.11 as well. |
4 participants
From the bug description there are two issues:
Just met the issue one time after trying at least ten times. This requests the copy action taking place right after the secret is created and added as webhook.
Haven't found any reasonable precedence for showing warnings in the description list terms in the codebase(hopefully I haven't overlooked anything), so created a proposal that would cover both issues:
Marked the PR as WIP to open the design discussion, code is not ready for review...
cc'ing @openshift/team-ux-review
@spadgett PTAL