New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1789979: Fix webhook triggers hooks to re-run on prop change #3929
Conversation
@jhadvig: This pull request references Bugzilla bug 1789979, 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. In response to this:
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. |
@@ -19,7 +19,7 @@ enum TriggerTypes { | |||
GitLab = 'GitLab', | |||
ImageChange = 'ImageChange', | |||
} | |||
const webhookTriggers = new Set<TriggerTypes>([ | |||
const webhookTriggersTypes = new Set<TriggerTypes>([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const webhookTriggersTypes = new Set<TriggerTypes>([ | |
const webhookTriggerTypes = new Set<TriggerTypes>([ |
@@ -90,9 +95,9 @@ export const WebhookTriggers: React.FC<WebhookTriggersProps> = (props) => { | |||
setWebhookSecrets(_.compact(secrets)); | |||
setLoaded(true); | |||
}); | |||
}, [isLoaded, canGetSecret]); | |||
}, [webhookTriggers, isLoaded, canGetSecret, namespace]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is going to re-fetch all the secrets on any build config change (not just on changes to triggers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spadgett tested this use-case and BC1 was not re-fetched if I updated BC2 (with adding trigger or build strategy parameters)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean any change to the current build config. For instance, I believe it will fetch all secrets again if, say, an annotation changes.
93d5244
to
fc4866c
Compare
@spadgett I've updated the PR. To prevent re-fetching of the secrets when any non-trigger changes are made we'll compare the next and previous webhookTriggers. If they match no changes are made to the webhookTriggers and so no secrets will be fetched. Otherwise fetch all the webhookTrigger secrets. |
@spadgett @TheRealJon comments addressed. PTAL |
return; | ||
} | ||
setWebhookTriggers(newWebhookTriggers); | ||
}, [triggers, webhookTriggers]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will adding webhookTriggers as a dep cause issues here since it is updated within this effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not since the webhookTriggers
will be updated only when the webhookTriggers are update(added/removed)
const newWebhookTriggers: WebhookTrigger[] = _.filter(triggers, ({ type }) => | ||
webhookTriggerTypes.has(type), | ||
); | ||
if (_.isEqual(newWebhookTriggers, webhookTriggers)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend using functional form of setState here, that way we don't have to have webHookTriggers as a dep in this effect. https://reactjs.org/docs/hooks-reference.html#functional-updates
React.useEffect(() => {
setWebhookTriggers((previousTriggers) => {
const newTriggers = _.filter(triggers, ({ type }) => webhookTriggerTypes.has(type));
return _.isEqual(previousTriggers, newTriggers) ? previousTriggers : newTriggers;
});
}, [triggers]);
@TheRealJon thanks for the suggestion. PR updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, TheRealJon 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:
Approvers can indicate their approval by writing |
@jhadvig: All pull requests linked via external trackers have merged. Bugzilla bug 1789979 has been moved to the MODIFIED state. In response to this:
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. |
/assign @spadgett