Tails

#1 Updated by intrigeri 2013-10-04 05:25:38

• Category set to Infrastructure
• Starter set to No

#2 Updated by intrigeri 2013-12-29 04:09:45

• Starter changed from No to Yes

#3 Updated by intrigeri 2014-03-29 20:55:25

• Description updated

#4 Updated by intrigeri 2014-04-29 05:22:11

• Description updated

#5 Updated by intrigeri 2017-03-26 11:13:27

• Description updated

#6 Updated by intrigeri 2017-03-26 11:24:12

• Description updated

#7 Updated by intrigeri 2017-03-26 11:29:52

• Description updated

#8 Updated by intrigeri 2017-03-26 11:31:38

• Description updated

#9 Updated by groente 2017-04-11 12:16:22

• File changenote.tar added

Here’s a unitfile, defaults file and a little script that should get the job done, provided inoticoming and an mta are installed on the system.

#10 Updated by intrigeri 2017-04-11 12:24:01

• Status changed from Confirmed to In Progress
• Assignee set to intrigeri
• % Done changed from 0 to 10
• QA Check set to Ready for QA

#11 Updated by intrigeri 2017-05-01 08:00:53

• Assignee changed from intrigeri to groente
• % Done changed from 10 to 20
• QA Check changed from Ready for QA to Dev Needed

> Here’s a unitfile, defaults file and a little script that should get the job done, provided inoticoming and an mta are installed on the system.

Thanks! Here’s an initial review. I don’t expect you to implement all the changes I suggest, if you can state good reasons why things are done in the way they currently are: this is a conversation, not me giving orders :)

• This looks good. In particular, it’s good that you used parameters for things that may change depending on the deployment environment.
• Please use /bin/sh unless there’s a good reason to use bashisms in the script (and thus /bin/bash in the shebang).
• Please use set -eu that help detect many small programming or configuration issues, e.g. $MAILTO being unset.
• It would be nice if the script had some basic sanity checks for all variables it expects, e.g. [ -n "$MAILTO" ].
• Can’t $MAIL be quoted in echo -e $MAIL? (I’m unsure about the interaction with echo -e, it might be that double-quotes wouldn’t work as I expect, but if they do, then please add them.)
• I’d rather not introduce a new manually managed log file (whose content is not in the systemd Journal, and that would need a new logrotate config snippet). Printing to stdout should be enough (thanks to --foreground) to have logs included in the systemd Journal.
• I suggest renaming changenote.sh to reprepro-notify-incoming-changes, and similarly changenote.service to reprepro-notify-incoming-changes.service: more explicit names make things easier to track and find.
• What’s the purpose of Alias=inoticoming.service?
• Have you verified that two instances of inoticoming that monitor the same directory and suffix both trigger actions when a file lands in the monitored directory? I would hope so, but it would be nice to be 100% sure it works :)
• We added support to the reprepro Puppet module for managing multiple APT repositories, so ideally the systemd unit would be a template one, instantiated for each managed APT repo: see “Optionally, units may be instantiated from a template file at runtime” in systemd.unit(5). But we don’t need that yet, so take this as a mere suggestion, and work on it only if you are excited at the idea of learning more about systemd :) I’d rather see you first polish the current version so that we can deploy it in production, and then work on this later if/whenever you feel like it.

#12 Updated by groente 2017-05-01 09:56:45

• File reprepro-notify-incoming-changes.tar added
• Assignee changed from groente to intrigeri
• % Done changed from 20 to 30

Here’s an updated version, renamed to reprepro-notify-incoming-changes:

- added some sanity checks

- replaced /bin/bash by /bin/sh and replaced the echo’s with printf to avoid troubles with (non-)posix-compliant shells

- replaced writing to a logfile with writing to stdout
- removed the Alias from the unit file.

Two instances can indeed monitor the same directory without interfering with eachother.

#13 Updated by groente 2017-05-01 09:57:22

• QA Check changed from Dev Needed to Ready for QA

#14 Updated by intrigeri 2017-05-03 08:19:10

• Target version set to Tails_3.0~rc1

#15 Updated by intrigeri 2017-05-19 20:18:13

• Target version changed from Tails_3.0~rc1 to Tails_3.0

Sorry!

#16 Updated by intrigeri 2017-05-28 09:08:06

• Assignee changed from intrigeri to bertagaz

I’ve asked bertagaz over email to take over the review of one of Feature #5894 and Feature #11523; I think this one will probably be the easiest for him to grasp so I’ll focus on Feature #11523 myself. You’ll want to read the previous review round comments above. Once happy, someone will have to Puppet-ize and deploy this, so we can evaluate how it fares in the real world; I can do it if you don’t want to :)

#18 Updated by bertagaz 2017-06-12 12:24:58

• Assignee changed from bertagaz to groente
• Target version changed from Tails_3.0 to Tails_3.1
• QA Check changed from Ready for QA to Dev Needed

bertagaz wrote:
> Thanks! Sorry groente for the delay in the reviewing. I’m quite busy with the changes in the build system, but I hope to be able to review this ticket at the end of the week, or next week if I don’t manage to do it before.

Had time to look at this and test it. I have only a few remarks:

• Please don’t always use the same exit code in the variables sanity checks. We usually use one for each (> 1) and increment. It helps a bit to debug what went wrong.
• Why are you adding ';' at the end of each lines inside this if … fi?
• Please quote the variables you’re testing there also.

Other than that looks good! I’ve tested it locally and it works fine, including when several services are watching the same directory indeed. I’ll puppetize this once you’ll have fixed what’s been raised above, and will point you to this puppet code.

#19 Updated by groente 2017-06-12 13:35:51

• File reprepro-notify-incoming-changes added

Thanks! Attached is an updated version of the script with incremental exit codes, less semicolons (that was just force of habit), and some extra quotes.

#20 Updated by groente 2017-06-12 13:36:25

• Assignee changed from groente to bertagaz
• % Done changed from 30 to 40

#21 Updated by intrigeri 2017-06-12 15:12:15

• QA Check changed from Dev Needed to Ready for QA

(Adjusting metadata accordingly.)

#22 Updated by bertagaz 2017-06-14 13:01:32

• QA Check changed from Ready for QA to Dev Needed

groente wrote:
> Thanks! Attached is an updated version of the script with incremental exit codes, less semicolons (that was just force of habit), and some extra quotes.

Ok, that’s better! Note that you’ve set the exit codes starting from “1”, where I asked them to be “> 1”, but that’s fine, I’ll adapt them when I’ll import your code. Just wanted you to know for the next time. :)

I’ll puppetize this later, and will point you to the puppet code.

#24 Updated by intrigeri 2017-06-22 15:35:52

• Priority changed from Normal to Elevated

The earlier this is done, the better wrt. the process we’re in with groente.

#26 Updated by bertagaz 2017-06-27 10:50:15

• Assignee changed from bertagaz to intrigeri
• % Done changed from 40 to 60
• QA Check changed from Dev Needed to Ready for QA
• Feature Branch set to puppet-tails:feature/5894-reprepro-notify-incoming-changes

intrigeri wrote:
> > where I asked them to be “> 1”, but that’s fine,
>
> Just curious: why?

Because other commands in the script (like the printf one in this case) may return “1” if they fail, so it does not mix the exit code we set up with the one that may arise from other commands (even if I know that this commands may also return something else).

Anyway, I’ve just pushed commit 65811f3 and commit 989ec075 in a dedicated branch. Did not test or deploy it yet, but I believe it’s ready to be reviewed now.

#27 Updated by intrigeri 2017-06-27 14:26:38

• Assignee changed from intrigeri to bertagaz
• QA Check changed from Ready for QA to Dev Needed

Looks good, except:

• mail_to => 'tails-sysadminsboum.org’@ ← uh, why would sysadmins be responsible for monitoring what’s essentially code changes? I would says RMs to start with, and possibly tails-dev@ later once we know it works fine.
• mail_from => 'repreproapt.lizard’@: won’t the internal FQDN make the email be rejected by the destination MTA?

Feel free to fix those and then deploy directly at a time when you’re ready to 1. immediately test that it indeed works as expected; 2. deal with the fallout (if any :)

#28 Updated by bertagaz 2017-06-28 10:16:28

• Assignee changed from bertagaz to intrigeri
• % Done changed from 60 to 70
• QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> Looks good, except:
>
> * mail_to => 'tails-sysadminsboum.org’@ ← uh, why would sysadmins be responsible for monitoring what’s essentially code changes? I would says RMs to start with, and possibly tails-dev@ later once we know it works fine.
> * mail_from => 'repreproapt.lizard’@: won’t the internal FQDN make the email be rejected by the destination MTA?

Makes sense. Fixed that.

> Feel free to fix those and then deploy directly at a time when you’re ready to 1. immediately test that it indeed works as expected; 2. deal with the fallout (if any :)

Done. We’ve just received the first notification while I uploaded a new package for Bug #12696. Yay!

Shall I change the recipient now that we know it works?

#29 Updated by intrigeri 2017-06-28 10:27:34

• Assignee changed from intrigeri to bertagaz
• % Done changed from 70 to 80
• QA Check changed from Ready for QA to Dev Needed

Great! :)))

Three comments:

• “this little daemon just saw the following file appear” is too verbose for an automated message we’ll receive pretty often
• “on apt” is not very useful info as is, IMO it’s either too much or too little: better use the FQDN
• I would appreciate being given a little bit more info, at least the target dist (e.g. we might want to detect issues like uploading straight to a release branch outside of the release process, or uploading to a tag). IMO just cat’ing the .changes file is the simplest option to start with, that should be good enough.

> Shall I change the recipient now that we know it works?

40 minutes of runtime + 1 single test email is not enough to make me comfortable targetting a public mailing list, given our history wrt. reacting in a timely manner to problems caused by such automated messages.

I say fix the 3 issues raised above, postpone to 3.2, wait until the 3.1 release process (when you’ll be uploading a number of packages), and redirect to tails-dev@ once 3.1 is out (+ of course explain tails-dev@ what’s happening). Fair enough?

#30 Updated by intrigeri 2017-06-29 09:54:55

• blocks Feature #13232: Core work 2017Q2: Sysadmin (Maintain our already existing services) added

#31 Updated by bertagaz 2017-06-29 14:45:40

• Priority changed from Elevated to Normal
• Target version changed from Tails_3.1 to Tails_3.2

intrigeri wrote:
> Great! :)))
>
> Three comments:
>
> * “this little daemon just saw the following file appear” is too verbose for an automated message we’ll receive pretty often
> * “on apt” is not very useful info as is, IMO it’s either too much or too little: better use the FQDN
> * I would appreciate being given a little bit more info, at least the target dist (e.g. we might want to detect issues like uploading straight to a release branch outside of the release process, or uploading to a tag). IMO just cat’ing the .changes file is the simplest option to start with, that should be good enough.

Fixed, pushed in the master branch of the puppet-tails Git repo (see diff and deployed.

> I say fix the 3 issues raised above, postpone to 3.2, wait until the 3.1 release process (when you’ll be uploading a number of packages), and redirect to tails-dev@ once 3.1 is out (+ of course explain tails-dev@ what’s happening). Fair enough?

Ack.

#32 Updated by intrigeri 2017-07-04 08:30:32

• blocks Feature #13233: Core work 2017Q3: Sysadmin (Maintain our already existing services) added

#33 Updated by intrigeri 2017-07-04 08:30:37

• blocked by deleted (Feature #13232: Core work 2017Q2: Sysadmin (Maintain our already existing services))

#34 Updated by bertagaz 2017-08-12 13:43:37

• Assignee changed from bertagaz to intrigeri
• QA Check changed from Dev Needed to Info Needed

bertagaz wrote:
> Fixed, pushed in the master branch of the puppet-tails Git repo (see diff and deployed.

Huhu, I noticed that I got the printf string wrong, and we now only receive the content of the changelog. OTOH I find it provides all the informations we need, so I wonder if we should not just do that and remove the extra bla lines of output in the email.

> > I say fix the 3 issues raised above, postpone to 3.2, wait until the 3.1 release process (when you’ll be uploading a number of packages), and redirect to tails-dev@ once 3.1 is out (+ of course explain tails-dev@ what’s happening). Fair enough?

I wonder though if tails-dev@ is not a too broad audience. Maybe the RMs and the package uploader is enough?

#35 Updated by intrigeri 2017-08-13 15:35:59

• Assignee changed from intrigeri to bertagaz
• QA Check changed from Info Needed to Dev Needed

> Huhu, I noticed that I got the printf string wrong, and we now only receive the content of the changelog. OTOH I find it provides all the informations we need, so I wonder if we should not just do that and remove the extra bla lines of output in the email.

Can you please paste here one example of the current email content, and one example of what you’re proposing?

Also, did you mean “changelog” or “.changes file”?

>> > I say fix the 3 issues raised above, postpone to 3.2, wait until the 3.1 release process (when you’ll be uploading a number of packages), and redirect to tails-dev@ once 3.1 is out (+ of course explain tails-dev@ what’s happening). Fair enough?

> I wonder though if tails-dev@ is not a too broad audience. Maybe the RMs and the package uploader is enough?

On the one hand, we don’t notify tails-dev@ when we merge Git branches so it would feel weird to start notifying that list for package uploads. OTOH it’s easy to track Git activity, but currently essentially impossible to track changes in our custom APT repo unless you receive the email we’re talking about here. I’m not sure what’s best. Given I’m proposing on Feature #6220 that we kill our custom APT repo (at least for the bits that affect the content of our ISO image), let’s not waste too much time debating/optimizing things here ⇒ your call! If you think RMs + uploader is best, fine with me, please go ahead :)

#40 Updated by bertagaz 2017-09-19 10:32:45

• Assignee changed from bertagaz to intrigeri
• QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> Great, we’re on the same page. So all that’s left to do here is fixing the email recipient.

Yep, and fix the bug that turn the script into only sending the .changes file. I’ve done that in two new commits on top of the related branch. Please review.

#41 Updated by intrigeri 2017-09-19 15:32:42

• Assignee changed from intrigeri to bertagaz

> Yep, and fix the bug that turn the script into only sending the .changes file. I’ve done that in two new commits on top of the related branch. Please review.

Looks OK, merged. Please test (IIRC we have a dedicated dist for testing stuff) and close once it’s confirmed to work fine.

#42 Updated by anonym 2017-09-28 18:29:21

• Target version changed from Tails_3.2 to Tails_3.3

#43 Updated by intrigeri 2017-10-01 10:01:25

• blocks Feature #13242: Core work: Sysadmin (Maintain our already existing services) added

#44 Updated by intrigeri 2017-10-01 10:01:28

• blocked by deleted (Feature #13233: Core work 2017Q3: Sysadmin (Maintain our already existing services))

#45 Updated by bertagaz 2017-10-03 10:55:57

• Status changed from In Progress to Resolved
• Assignee deleted (bertagaz)
• % Done changed from 80 to 100
• QA Check changed from Ready for QA to Pass

intrigeri wrote:
> Looks OK, merged. Please test (IIRC we have a dedicated dist for testing stuff) and close once it’s confirmed to work fine.

It does, closing.