Include playbook url into mail #105
Conversation
There was a problem hiding this comment.
This would certainly do the trick, but I find it rather ugly.
How about modify the SendNotification method to accept a template (if nil just use the text string) and provide the template as some kind of config. The config then can be set with an env variable.
|
I am a bit hesitant to modify the signature of SendNotification, as it is used not only in the AddUnknownClientId case, thus, I keep it as is and just extracted the the logic to the separate method. @JHK please have a look whenever you have time |
go/server_state.go
Outdated
| func getNotification(defaultMessage string) string { | ||
| templateString, exists := os.LookupEnv("UNKNOWN_CLIENT_ID_MESSAGE_TEMPLATE") |
There was a problem hiding this comment.
I still think the signature of this method is too narrow. It is there to serve just a very specific use-case, but there are some other occurrences sending messages where a template could make sense.
I understand the idea of not changing the SendNotification method and agree. It is already simple enough and fulfills one purpose.
The getNotification method is very tailored to the exact occurrence of the one environment variable. This variable is hidden deeply in the code and can only be used when you know it upfront.
I would suggest a different interface:
- The beetle binary gets parameters to set the name of envionment variables (or files?) to read the templates from
- Those pointers are stored in the
ServerOptsstruct (a sub struct) - The
getNotificationmethod gets another parameter, where you can put the pointer to the template reference - The
getNotificastionmethod resolves the template (if it exists) and renders it
This would mean the configuration parameter becomes visible to the outside and the implementation becomes more extensible.
|
@JHK please have another look whenever you have time |
|
@danielgoncharov is this still something that you would like to get in? |
|
Lets close it, we will deprecate the whole thing anyway soon |
No description provided.