-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
Added custom notification messages using text/template #60
Conversation
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.
This is really good, thank you! I tested this and it works well for me.
I left some comments inline, let me know what you think.
My initial idea about approaching this would be the following: instead of having type stats struct {
Start time.Time
Output *bytes.Buffer
PrunedBackups int
// whatever we want to add ...
}
type script struct {
// script holds all the stateful information required to orchestrate a
// single backup run.
type script struct {
cli *client.Client
minioClient *minio.Client
webdavClient *gowebdav.Client
logger *logrus.Logger
sender *router.ServiceRouter
template *template.Template
hooks []hook
hookLevel hookLevel
file string
stats *stats
c *config
} Any method on |
I agree, that's the approach I was going to suggest. One thing: basically all the propertis in the
But what happens to the |
That's a good question. The template package will try to somehow stringify anything it sees using reflection, and right now I'm not sure how it will represent a buffer (it could either be turned into an UTF-8 string or into a set of bytes, i.e. octal representation). After trying it out, it seems it picks the human readable version though, so I think we could do this: https://go.dev/play/p/aw4XCMH4GjD |
As you can see from the two commits I've implemented the discussed changes. From my testing everything works fine. I'll also share my success template:
|
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.
This is going to be brilliant. Thanks for putting so much thought into this, it's really appreciated.
I left a few comments inline once more, nothing really major though.
We'll also need docs for this I guess. Do you think a new section in How to
would be enough considering we'll have to document the entire NotificationData
type?
I think the best way is to add a section in the What do you think? |
This probably depends on how much docs end up being written, but it definitely sounds like a good start. Once there is content, it can always be restructured. |
I have took care of all the issues you mentioned. Also, I have drafted the initial documentation for the notification templates. Let me know what do you think :) EDIT: the tests have failed. Is it possible that the reason is that I've removed the sleep in the pruning, thus not leaving docker enough time to start up the containers before the check is made? |
Considering
I think it's very likely this is a race condition created by removing the docker-volume-backup/test/swarm/run.sh Lines 26 to 30 in 3bb99a7
I'll have a close look at the PR itself later today. |
Agreed. Do you think 5 seconds is enough? Looking at the test directory, I think the right place is just before these three lines:
Is it correct? Or just the line in |
I think we only need to add it for the
That being said: it could be argued that the compose test should check that containers are up before it looks at the artifacts, which would probably also require the sleep to be introduced. Long story short: it would be great if you could add the sleep for the swarm test and I'll see if I can clean up and also document the tests a little better soon (I'd also like to add a test for the notifications feature at some point, but this will be a little tricky). |
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.
This turned out really well. I left some minor suggestions, feel free to adjust or disregard these as you wish. If we can also get CI green again I would say this one is ready to go 🎉
It seems to be fixed with the sleep. I've also implemented the other changes. |
This is now released in |
Thanks to you for the project and your quick to the point assistance 🔥 |
This should take care of #59. This is my first time ever with Golang so I'm sorry if I've done anything not in the correct way.
As per your suggestion, I've used the
text/template
package. The default template uses the exact same wording of current notifications.A few things:
StartTime
,LogOutput
.BackupStopContainerLabel
(andError
for failures). I would have liked to add more (like number of stopped containers, number of pruned backups, took time, and many more), but some things would have required too much refactor of the code to have this variables ready in thenotifySuccess
function. Any thoughts on this?