-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Support templating in webhook url fields #4798
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
base: main
Are you sure you want to change the base?
Support templating in webhook url fields #4798
Conversation
Signed-off-by: Walther Lee <walthere.lee@gmail.com>
SoloJacobs
left a comment
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.
From what I understand:
- We don't expose
MarshallYAML, so the new implementation is backwards compatible. I.e, all old configurations are accepted by the newUnmarshallYAMLimplementation. Here are two configurations, which are only accepted by the new implementation.
- name: 'empty-url'
webhook_configs:
- url: ''
url_file: 'url.txt'
- name: 'not-a-url'
webhook_configs:
- url: 'ssh://ab'For the not-a-url config the new behaviour is rather unfortunate. Did you consider validating URLs, which don't have a template?
- Could we maybe get some tests for the new templating in
notify/webhook/webhook.go? - There are also some other configurations, which could be handled in a similar manor I think:
SlackConfigMSTeamsConfigMSTeamsV2ConfigMattermostConfigDiscordConfig
I guess that is out-off-scope for this PR, though.
|
Sure, I can add the change for those too and some tests. Thanks Solomon |
Signed-off-by: Walther Lee <walthere.lee@gmail.com>
Signed-off-by: Walther Lee <walthere.lee@gmail.com>
|
The change from I added tests for templated urls in webhook but won't change the other URL fields from other configs in this PR to keep it small. I'll send a second one with just the switch to |
Signed-off-by: Walther Lee <walthere.lee@gmail.com>
It's an old thread, but there's been some discussion here about supporting templating in webhook url fields: #684
The issue was closed in 2017 because the alertmanager didn't support templating for any http/url fields, but there're some examples where it does support it now, for example SlackAction.URL and PushoverConfig.URL, so I think we can re-evaluate adding support for this.
There's some good use cases described by others in the thread since it was closed.
This change adds templating to the fields. Existing tests already check that secret URL is still not exposed.