search

jeffaco / duplicacy-util

Utility to schedule and run duplicacy backup via the command line
Apache License 2.0
94 stars 24 forks source link

Groundwork for multiple notification channels #14

Closed boedy closed 6 years ago

boedy commented 6 years ago

As discussed in https://github.com/jeffaco/duplicacy-util/issues/10. I'll be updating this pull request until all todo's are checked off.

Changelog

jeffaco commented 6 years ago

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

boedy commented 6 years ago

@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 commented 6 years ago

@boedy I have multiple reasons for this:

  1. 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.

  2. 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.

  3. 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.

  4. 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.

  5. 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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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

Back to you 👉

jeffaco commented 6 years ago

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 commented 6 years ago

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.