feat: refresh ApplicationSet resource with git generator using webhook #341
Conversation
chetan-rns
force-pushed
the
git-webhook
branch
from
5ed7aa3
to
6bbbace
Compare
|
@chetan-rns If you rebase to master it should fix the E2E tests, FYI |
Currently, the repository used in git-generator will be polled every 3 mins. This commit adds support to refresh the applicationset using a git webhook. It exposes a service listening for incoming webhook payloads and triggers the applicationset controller.
chetan-rns
force-pushed
the
git-webhook
branch
from
6bbbace
to
d267799
Compare
There was a problem hiding this comment.
Looks great, thanks @chetan-rns! Only one change requested:
Can you create a doc page (in /docs) for the webhook feature? You can probably just base it on the first step of the Argo CD Web Hook doc page. Content looks like it should basically be the same between the two (and we probably only need the first step from there).
chetan-rns
force-pushed
the
git-webhook
branch
from
5d11b17
to
64d7774
Compare
There was a problem hiding this comment.
Content looks great! Rather than having webhook be its own page (like it is for Argo CD docs), let's move the contents of the new Webhook page into the existing Generators-Git.md file, adding it to the end of the document, eg:
## Git Generator: Files
(... previous content of Git Generator: Files...)
## Git Generator Webhook Configuration
When using a Git generator, ApplicationSet polls Git repositories (...)
(At first I thought we should have it be a separate page (like with Argo CD, and in your last commit) but I think it makes sense to include this content on the Git generator page, as it is applicable to both generator types, but is only applicable to these two generators. )
chetan-rns
force-pushed
the
git-webhook
branch
from
64d7774
to
b23339a
Compare
|
@jgwest I agree it makes more sense to have the webhook section in the existing Git Generator page. I've updated it. Please take a look :) |
There was a problem hiding this comment.
Awesome, thanks @chetan-rns! Really great to get this feature in, which folks have been looking for 👍 .
| func startWebhookServer(webhookHandler *utils.WebhookHandler) { | ||
| mux := http.NewServeMux() | ||
| mux.HandleFunc("/api/webhook", webhookHandler.Handler) | ||
| go func() { | ||
| setupLog.Info("Starting webhook server") | ||
| err := http.ListenAndServe(":7000", mux) | ||
| if err != nil { | ||
| setupLog.Error(err, "failed to start webhook server") | ||
| os.Exit(1) | ||
| } | ||
| }() | ||
| } |
There was a problem hiding this comment.
Hmm, is it a good idea to hardcode the port here as other port bindings are configurable?: https://github.com/argoproj-labs/applicationset/blob/master/main.go#L84-L85
flag.StringVar(&metricsAddr, "metrics-addr", ":8080", "The address the metric endpoint binds to.")
flag.StringVar(&probeBindAddr, "probe-addr", ":8081", "The address the probe endpoint binds to.")Also it would be nice to make the webhook handler optional. This introduces a hard dependency on the ConfigMap argocd-cm which makes it really hard to find a good solution for the helm chart update to 0.3.0:
argoproj/argo-helm#1070
There was a problem hiding this comment.
Hi @mkilchhofer, thanks for bringing this up. I agree the webhook address should be configurable. I've raised a PR for this #450. It also makes the webhook handler optional by removing the hard exit.
There was a problem hiding this comment.
Cool. Also worked on this :-D but you were faster, thanks. 🙏
Currently, the repository used in git-generator will be polled every 3 mins. This commit adds support to refresh the application set using a git webhook. It exposes a service listening for incoming webhook payloads and triggers the application set controller.
Fixes: #222