-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
feat(hydrator): handle sourceHydrator fields from webhook #19397
Conversation
1d7fbe3
to
a76bcb6
Compare
Signed-off-by: daengdaengLee <gunho1020@gmail.com>
e3f59a6
to
4b3767f
Compare
Hello, @crenshaw-dev . |
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.
Thanks so much for all the work and for the rebase! Looks like we had some similar ideas over the weekend. :-)
Do you have time to add some tests for apps with source hydrators?
util/webhook/webhook.go
Outdated
if path.AppFilesHaveChanged(refreshPaths, changedFiles) { | ||
namespacedAppInterface := a.appClientset.ArgoprojV1alpha1().Applications(app.ObjectMeta.Namespace) | ||
log.Debugf("webhook trigger refresh app to hydrate '%s'", app.ObjectMeta.Name) | ||
_, err = argo.RefreshApp(namespacedAppInterface, app.ObjectMeta.Name, v1alpha1.RefreshTypeNormal) |
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.
Should we modify the argo.RefreshApp
function to allow setting just the hydrate annotation instead of the refresh annotation?
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.
Since the app's features are functioning properly, I think it's fine as it is.
If I understand correctly, when there’s an update (patch to k8s, and listen that from informer), messages are added to both the appRefreshQueue
(roughly) and appHydrateQueue
, and the subscribers to each queue check the execution conditions after receiving the messages.
So even if we make something like argo.HydrateApp
, it shouldn't make a significant difference in how the messages are handled.
If I misunderstood something, could you correct me in a comment pleaase?
Thanks!
Signed-off-by: daengdaengLee <gunho1020@gmail.com>
Hello, @crenshaw-dev. Sure! I can add some tests.
|
Yep, tests in this PR would be great! I'd just copy some of the test cases in webhook_test.go and use a SourceHydrator in the spec instead of a Source. |
… SourceHydrator is configured Signed-off-by: daengdaengLee <gunho1020@gmail.com>
Hello @crenshaw-dev. Thank you for letting me know there are tests for webhook. Please let me know if I missed anything. |
Hello, @crenshaw-dev . Could you check this PR please? Thank you. |
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, thank you!
Hello. This is a PR resolves #19070.
I added logging when the new code was executed. I confirmed that the logs were recorded when the webhook was triggered.
If you notice anything I missed, please let me know in the comments, and I'll make the necessary corrections!
Thanks.
Checklist: