fix: nil pointer on notifier #504
Conversation
Codecov Report
@@ Coverage Diff @@
## master #504 +/- ##
==========================================
- Coverage 52.17% 52.08% -0.10%
==========================================
Files 60 61 +1
Lines 4529 4537 +8
==========================================
Hits 2363 2363
- Misses 1770 1778 +8
Partials 396 396
Continue to review full report at Codecov.
|
|
https://github.com/weaveworks/flagger/blob/nop-notifier/cmd/flagger/main.go#L329 there's still a possible branch for notifier being nil... 😞 |
|
@mathetake you are right, I made a mess out of the global notifier when I added env vars support. |
|
@stefanprodan thank you for quick response 👍 |
|
@mathetake I think the NopNotifier shouldn't return an error and just be nop. This way the |
got it |
| client, err = notifierFactory.Notifier(provider) | ||
| if err != nil { | ||
| logger.Errorf("Notifier %v", err) | ||
| } else if len(notifierURL) > 30 { |
There was a problem hiding this comment.
@stefanprodan i am not sure what 30 means. could you tell me what is the purpose for slicing in the logging🤔 ?
There was a problem hiding this comment.
I wanted to avoid logging secrets as the Slack/Teams URL will contain the API Key. But this is poorly done since the URL can be shorter for other providers (for example a self-hosted Rocket) and the info will never get logged.
resolves #496
if slack's url is configured through
SLACK_URLenvironment variable without providingslack.url, current helm chart ignoresslack.channelandslack.user, which results in notifier in the controller struct becoming nil.just in case, i added
nopNotiferfor warning users about misconfigurations in notifiers.