Merge ~bryce/git-ubuntu:sendmail-object into git-ubuntu:master

PASSED: Continuous integration, rev:6e038ffe18f94cc25285e0c0cd0ccb5607021a27
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/402/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/402//rebuild

PASSED: Continuous integration, rev:b361f4b892380b8f9306ff351e2a9cd58f574607
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/403/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/403//rebuild

This looks good and is a welcome move in moving code into the module, encapsulating the email notification behavior and state and adding tests.

I'm not keen on tests relying on time.time() because this approach is generally prone to races, even though I don't see any on this occasion. Can you rearrange to avoid calls to time.time() during test runtime (both in test code and the code under test)?

I usually use this pattern:

def something_that_uses_time(..., time_func=time.time):
   # access time
   time_func()

Then in production code the library function can be used as normal, but tests can easily control the passage of time directly, for example with a return_value or side_effect attribute on a Mock object.

More comments inline.

Thanks, I've incorporated the review feedback, and verified the tests still pass. Please have another look when you have a chance.

PASSED: Continuous integration, rev:efea26f7571a4d39759ac172b60429daee04d12d
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/431/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/431//rebuild

I don't see review feedback incorporated. Did you push the right branch? I'm looking at efea26f. Here are the changes I see against what I reviewed previously:

$ git range-diff d358c19..bryce/sendmail-object.v1 9e8e5da..bryce/sendmail-object
1: bb91757 = 1: 88a6898 Refactor importer to encapsulate sendmail logic to its own object
2: 6e038ff = 2: 4a546e4 Send fewer package import/failure emails
3: b361f4b ! 3: efea26f systemd: Include last 100 lines from journal in system failure emails
    @@ -2,6 +2,11 @@

         systemd: Include last 100 lines from journal in system failure emails

    + Log errors from this script to the journal as well.
    +
    + Also, drop unnecessary User/Group settings in service, since this runs
    + fine as the current (non-root) user.
    +
     diff --git a/bin/failure-email.sh b/bin/failure-email.sh
     --- a/bin/failure-email.sh
     +++ b/bin/failure-email.sh
    @@ -19,3 +24,17 @@
     +$(journalctl --unit=usd-importer.service -n 100 --no-pager)
      EOF

    +
    +diff --git a/usd-importer-failure-email@.service b/usd-importer-failure-email@.service
    +--- a/usd-importer-failure-email@.service
    ++++ b/usd-importer-failure-email@.service
    +@@
    +
    + [Service]
    + Type=oneshot
    +-ExecStart=/snap/git-ubuntu/current/bin/failure-email.sh %i
    +-User=nobody
    +-Group=systemd-journal
    ++ExecStart=/bin/bash /snap/git-ubuntu/current/bin/failure-email.sh %i
    ++StandardOutput=journal
    ++

Sorry, yes you're right I pushed the wrong branch. I've pushed the correct one now.

PASSED: Continuous integration, rev:b505af02e3bfa58d0762777ed9bbf31a81dbcf98
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/436/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/436//rebuild

Looks good, thanks!

A couple of minor corrections inline.

I also noticed in the diff against your previous version of the branch that some line lengths were busting the 79 limit. Looking now, there are some cases I missed before, sorry. Please could you adjust to keep to the line length limit?

Thanks, I've made the updates you requested and pushed the branch, per discussion at standup today.