Groundwork for multiple notification channels #14

boedy · 2018-09-12T21:48:36Z

As discussed in #10. I'll be updating this pull request until all todo's are checked off.

Changelog

Support for custom notification channels. Currently only email
Refactored duplicity-util.go (more central error handeling)
Validate config for valid notification channels
Global config testing
Check for config parsing errors
Add warning for old config usage

jeffaco · 2018-09-15T17:40:58Z

@boedy Let me know when this is ready for a code review. Please squash your commits before asking me to review. thanks!

boedy · 2018-09-15T18:46:55Z

@jeffaco Yes, I'll be able to do that if you want. What's the reason the commits should be squashed upfront? I usually only commit working encapsulated parts and feel the commit messages help with describing the intent of each change. I understand the desire to keep the git history of the master branch clean, but if I'm not mistaking github allows you to squash the commits when merging this PR.

As for the PR. I'm almost there I think. I'll let you know when it's ready for review 😄 !

jeffaco · 2018-09-15T19:49:53Z

@boedy I have multiple reasons for this:

I prefer using git command line tools to make changes. That way, I'm certain what the commit message(s) will look like before merge to master. Merges to master are harder to "undo", particularly when not caught immediately. I feel that good commit messages are as important as the changes themselves.
I'm very picky on commits for long term support purposes. I've been in the software development business for a long time. As for commit messages, I don't care what was changed (git show will let me see that), I care about why the change was made. This is often critical when looking back to a change a year or two earlier. This project is likely small enough that it doesn't matter, but I believe that's an important rule to follow in general, all the time. It's a great practice.
As you say: I like to keep the commit history "clean". It should be formatted to 70 bytes (or so) per line, it should make sense explaining why things were done, etc.
Commit messages should "stand alone". That is, I commit should make sense on it's own, without other commits around it. For example, it's expected to include unit tests in new features, so having an additional commit saying unit tests were added (unless it was done as a separate piece of work) is not necessary, and implies that you didn't include the unit tests the first time around.
Finally, minor commits (like ignoring a vim file), when done as part of a larger change, really need not be a separate commit. That, to me, is just noise. Now, of course, if you're using vim and no other changes are made around it, then it's a separate commit. Otherwise, it should just be included elsewhere.

So, if you can follow all that and it makes sense to have separate commit messages, I'm cool with that. For example, if you're refactoring mail test messages for support purposes, and you want that to be a completely separate commit, that's fine. But if it's contained in other work and mentioned in a multi-line commit, that's fine too. That's sort of up to you, as long as commit messages "stand alone" on their own and make sense.

Hope this makes sense!

boedy · 2018-09-15T20:30:58Z

I think I understand where you're coming from. I've just pushed my latest changes and squash / rewritten commit messages and in my opinion ready for review. Note I have not actually tested the email notifications. Might be good to still give that a try.

jeffaco

I haven't looked at README.md yet, there's a ton of churn there so it'll take a closer look. I did learn a few things from you about markdown formatting, though! 👍

Also, I haven't tested e-mail yet. I'll do that when I think we have something close to final.

In short, this is a GREAT job. I'm really happy with what you've done. There were some nits and some questions about how you went about testing the code. But other than that, this looks really good. Thanks!

configGlobal.go

configGlobal_test.go

duplicacy-util.go

emailNotifier.go

emailNotifier_test.go

jeffaco · 2018-09-16T01:11:26Z

Oh, by the way, on that first commit, please rewrite to get rid of the blank lines. Something like:

Added support for multi channel notifications

Added support for custom notifiers
Ingore gotags file
Updated config and first tests
Fixed linting error
Update README.md to reflect changes
Move sendTestmail function to sendmail.go
Cleaned up test
Throw error if invalid notification channel is set in global config
Abstracted notification channel configuration + added tests

would be great. Thanks again.

boedy · 2018-09-16T14:17:15Z

@jeffaco I made some new changes, but I'm just realising now, that previous comments will be lost if I force push my changes. What do you recommend?

jeffaco · 2018-09-16T16:41:42Z

The comments are still there - you just need to pull up historical comments. If you've addressed everything in my latest batch of comments, feel free to force push. In this case, you had some outstanding questions (which I just answered), so it was appropriate to wait for clarification. That way, I'm only reviewing changes once and not twice.

So address everything I said (unless you have followup questions), then then push again. Thanks!

boedy · 2018-09-16T19:21:41Z

@jeffaco I went ahead and gave it another shot. Along the way I discovered some things I had overlooked:

If no config was provided -m and -tm flags had no effect
duplicacy-util did not check for valid yml (i.e parsing errors)
-tn flag makes no sense if no notification are configured. It now throws an error it that's the case

Back to you 👉

jeffaco

I really like what you've done with the globalConfig tests, great job there!

Very minor nits here. Verify what I said, make any final changes, and bounce back to me. I'll take a final look, test without making any configuration changes (everything should still work), verify I get email, and then test one more time without -m and setting up the notifiers.

I probably won't be able to do this tonight, but should be able to do this tomorrow.

Thanks for the awesome job with this feature!

configGlobal.go

jeffaco

Looks good to me. Consider this approved for now; I'll do testing later tonight and then merge to master if all looks good. Thanks, again, for your contribution. Awesome job!

jeffaco

Hi Boedy,

I'm having some problems during testing. Please see branch boedy-notifications on the github repo.

First, I don't have duplicacy-util on my default path, and that causes the unit tests to fail. I have a workaround for that (I would have preferred to fix through some sort of dependency injection, but that's harder so I'll leave that for another day).

But more importantly: E-mail isn't working. If I don't change anything and use -m, I should get success and failure notifications. Instead, I was getting onStart and onFailure notifications (not success).

In the final commit (marked WIP), I made changes that should have fixed that, but strangely, it's not working, and it will take more detailed debugging for me to understand why. It's late, I'm tired, and I don't really have time for that right now, unfortunately.

Can you take a look at my final commit and see exactly why -mt (or just running duplicacy-util normally) won't send onSuccess notifications? If you don't have time, I'll look further later. But my thought is that you have time and understand your additions well enough to rapidly find the issue.

Thanks in advance.

Added support for custom notifiers Ingore gotags file Update README.md to reflect changes Move sendTestmail function to sendmail.go Throw error if invalid notification channel is set in global config Abstracted notification channel configuration (with tests) Add duplicacy executable stub for testing

Throw error if no notifiers are set during notification testing

Unit tests don't necessarily have a fully runnable environment. For example, there may not be "duplicacy" in the path. Add a framework to disable specific operations during unit testing.

boedy

I just configured email settings and tested all notifications types. All were successfully sent when using -tn, -tm, but when running a full backup with like you said success was never sent. Turned out there was no notifiyOfSuccess being called...
I tried both the old and new email configuration styles (both worked).

boedy · 2018-09-18T08:59:48Z

backup-ops.go

@@ -94,6 +97,9 @@ func performBackup() error {
 	logger.Println("######################################################################")
 	logMessage(logger, fmt.Sprint("Operations completed in ", getTimeDiffString(startTime, endTime)))

+	// Notify all configure channels that the backup process has completd
+	notifyOfSuccess()


My Apologies, you were right. Somehow I forgot to actually put this line here 😞.

jeffaco · 2018-09-18T16:45:24Z

This is very odd. I knew the onSuccess notifier wasn't called, but this was secondary. I couldn't get -tm to work properly for me at all (I just wasn't getting the success message). And running Go in debug mode on the Mac (no gdb) seemed a little difficult without moving to homebrew, etc.

But yesterday, from home, if I ran ./duplicacy-util -tm, I never got the success message (even with my new commit) on the notifications version, only the failure message. However, if I ran the last shipping version, -tm gave me exactly what I expected. Yet, from perusal of the code, I couldn't figure out why last night.

Any thoughts here? Or should I chalk it up to insanity, and try again? With no changes at all to the configuration file, I would expect:

/path/to/shipping/version/duplicacy-util -tm

and

./duplicacy-util -tm

to behave 100% identically.

jeffaco · 2018-09-18T17:05:22Z

Update: I'm clueless. I just tried -tm from Linux, and I got both success and failure e-mail messages right out of the gate. I'll try from the Mac again at home tonight. Hopefully all will work fine.

You may want to take a look at the final commit to insure it's correct: 41a57a9. Thanks.

jeffaco

So very odd. I just tested again, passed all tests (with both old and new config file formats). Merging now.

boedy mentioned this pull request Sep 12, 2018

Feature: Desktop notifications #10

Closed

3 tasks

boedy force-pushed the notifiers branch 2 times, most recently from 60fd4e9 to 3a00fdc Compare September 12, 2018 22:00

boedy changed the title ~~Groudwork for multiple notification channels~~ Groundwork for multiple notification channels Sep 13, 2018

jeffaco mentioned this pull request Sep 14, 2018

Feature request: Email function should also have an option to accept any SSL certificate #15

Closed

boedy force-pushed the notifiers branch from 0df8e3b to 352a893 Compare September 15, 2018 16:46

boedy force-pushed the notifiers branch from 352a893 to 9f84a33 Compare September 15, 2018 20:26

jeffaco requested changes Sep 16, 2018

View reviewed changes

boedy force-pushed the notifiers branch 4 times, most recently from 3771527 to 08ee464 Compare September 16, 2018 18:59

jeffaco requested changes Sep 16, 2018

View reviewed changes

configGlobal.go Outdated Show resolved Hide resolved

configGlobal.go Outdated Show resolved Hide resolved

boedy force-pushed the notifiers branch from 08ee464 to 65fff6f Compare September 17, 2018 20:12

jeffaco approved these changes Sep 17, 2018

View reviewed changes

jeffaco requested changes Sep 18, 2018

View reviewed changes

boedy force-pushed the notifiers branch from 65fff6f to 52cba26 Compare September 18, 2018 08:14

boedy added 5 commits September 18, 2018 10:55

Prevent duplicate notification handlers

69e9e13

Show warning when old config file is used

3578854

Deprecated -tm in favor of -tn (test notifications)

69e9aeb

Throw error if no notifiers are set during notification testing

Throw error if configuration fails to parse

4f41f2f

boedy and others added 2 commits September 18, 2018 10:55

Keep support for -m flag until version 2.0

f6d6153

Add concept of special behavior when running unit tests

762841e

Unit tests don't necessarily have a fully runnable environment. For example, there may not be "duplicacy" in the path. Add a framework to disable specific operations during unit testing.

boedy force-pushed the notifiers branch from 52cba26 to 762841e Compare September 18, 2018 08:55

boedy commented Sep 18, 2018

View reviewed changes

jeffaco approved these changes Sep 19, 2018

View reviewed changes

jeffaco closed this Sep 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Groundwork for multiple notification channels #14

Groundwork for multiple notification channels #14

boedy commented Sep 12, 2018 •

edited

Loading

jeffaco commented Sep 15, 2018 •

edited

Loading

boedy commented Sep 15, 2018

jeffaco commented Sep 15, 2018

boedy commented Sep 15, 2018 •

edited

Loading

jeffaco left a comment

jeffaco commented Sep 16, 2018

boedy commented Sep 16, 2018

jeffaco commented Sep 16, 2018

boedy commented Sep 16, 2018

jeffaco left a comment

jeffaco left a comment

jeffaco left a comment •

edited

Loading

boedy left a comment

boedy Sep 18, 2018

jeffaco commented Sep 18, 2018

jeffaco commented Sep 18, 2018

jeffaco left a comment

Groundwork for multiple notification channels #14

Groundwork for multiple notification channels #14

Conversation

boedy commented Sep 12, 2018 • edited Loading

jeffaco commented Sep 15, 2018 • edited Loading

boedy commented Sep 15, 2018

jeffaco commented Sep 15, 2018

boedy commented Sep 15, 2018 • edited Loading

jeffaco left a comment

Choose a reason for hiding this comment

jeffaco commented Sep 16, 2018

boedy commented Sep 16, 2018

jeffaco commented Sep 16, 2018

boedy commented Sep 16, 2018

jeffaco left a comment

Choose a reason for hiding this comment

jeffaco left a comment

Choose a reason for hiding this comment

jeffaco left a comment • edited Loading

Choose a reason for hiding this comment

boedy left a comment

Choose a reason for hiding this comment

boedy Sep 18, 2018

Choose a reason for hiding this comment

jeffaco commented Sep 18, 2018

jeffaco commented Sep 18, 2018

jeffaco left a comment

Choose a reason for hiding this comment

boedy commented Sep 12, 2018 •

edited

Loading

jeffaco commented Sep 15, 2018 •

edited

Loading

boedy commented Sep 15, 2018 •

edited

Loading

jeffaco left a comment •

edited

Loading