Update msteams webhook URLs to use mandatory secrets#2204
Update msteams webhook URLs to use mandatory secrets#2204jovial wants to merge 4 commits intostackhpc/2025.1from
Conversation
Current default never makes sense.
Alex-Welsh
left a comment
There was a problem hiding this comment.
LGTM
One suggestion but it's fine if you want to ignore it. I'm happy to merge as is
| - name: 'msteamsv2-notifications' | ||
| msteamsv2_configs: | ||
| - webhook_url: '{{ secrets_msteams_notification_channel_url | default('https://prod-01.westeurope.logic.azure.com/workflows/') }}' | ||
| - webhook_url: '{{ secrets_msteams_notification_channel_url | ansible.builtin.mandatory }}' |
There was a problem hiding this comment.
nit: it'd be nice to also include a message here to make it clear what the variable is and how to acquire one
| - webhook_url: '{{ secrets_msteams_notification_channel_url | ansible.builtin.mandatory }}' | |
| - webhook_url: "{{ secrets_msteams_notification_channel_url | ansible.builtin.mandatory(msg='secrets_msteams_notification_channel_url must be defined when xyz is enabled and can be found by looking in foo place.' }}" |
There was a problem hiding this comment.
Granted that is better than the standard:
fatal: [localhost]: FAILED! => {"msg": "Mandatory variable 'secrets_msteams_notification_channel_url' not defined."}
There was a problem hiding this comment.
Code Review
This pull request updates an example configuration file for Prometheus Alertmanager. The change replaces default, non-functional webhook URLs for Microsoft Teams notifications with a requirement that these URLs be provided as mandatory secrets. This is a good practice as it ensures users configure their specific endpoints, improving the robustness and security of the setup. The change is correct and well-implemented using Ansible's mandatory filter. I have no issues to report.
|
Darn, it looks like kayobe is copying the example file so needs the variable defined. I will try and prevent that, but probably explains why there was a default 😆 |
Current default never makes sense.