Tails

#2 Updated by intrigeri 2016-04-15 06:40:58

• blocked by Bug #11354: Disable Jenkins notifications on ISO build/test failure added

#4 Updated by intrigeri 2016-04-15 06:48:20

• blocks deleted (Bug #11354: Disable Jenkins notifications on ISO build/test failure)

#5 Updated by intrigeri 2016-04-15 06:48:40

• Parent task set to Feature #9614

#6 Updated by intrigeri 2016-04-15 06:48:55

• blocked by Bug #11354: Disable Jenkins notifications on ISO build/test failure added

#7 Updated by intrigeri 2016-04-25 02:41:37

• related to Feature #9719: Configure Jenkins notifications to our ticket tracker added

#8 Updated by bertagaz 2016-06-08 06:09:17

• Status changed from Confirmed to In Progress

As a first step, I’ll do some stats on the last months build run to see if we can at least re-enable notifications for them.

#10 Updated by bertagaz 2016-08-02 03:46:32

• Target version changed from Tails_2.5 to Tails_2.6

#11 Updated by intrigeri 2016-08-18 07:42:48

• related to Bug #10288: Fix newly identified issues to make our test suite more robust and faster added

#12 Updated by anonym 2016-09-20 16:53:52

• Target version changed from Tails_2.6 to Tails_2.7

#13 Updated by bertagaz 2016-10-03 20:57:14

• Target version changed from Tails_2.7 to Tails_2.9.1

#14 Updated by anonym 2016-12-14 20:11:19

• Target version changed from Tails_2.9.1 to Tails 2.10

#15 Updated by intrigeri 2016-12-18 10:02:48

• Target version changed from Tails 2.10 to Tails_2.11

#16 Updated by bertagaz 2017-03-08 10:38:04

• Target version changed from Tails_2.11 to Tails_2.12

#17 Updated by bertagaz 2017-03-08 13:24:39

• Priority changed from Normal to Elevated

#19 Updated by intrigeri 2017-04-20 07:12:26

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

Please ensure the new target version is more realistic than 2.12, after having looked at your 3.0~rc1 and 3.0 tickets. Also, IIRC you mentioned you already have had some WIP locally for a while, that you might want to push to a branch somewhere, so it’s not stored on your hard drive only, and others can help move this forward :)

#20 Updated by intrigeri 2017-05-19 10:16:19

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

intrigeri wrote:
> Please ensure the new target version is more realistic than 2.12, after having looked at your 3.0~rc1 and 3.0 tickets. Also, IIRC you mentioned you already have had some WIP locally for a while, that you might want to push to a branch somewhere, so it’s not stored on your hard drive only, and others can help move this forward :)

Ping :)

#21 Updated by bertagaz 2017-05-19 15:34:00

• % Done changed from 0 to 20
• Feature Branch set to puppet-tails:feature/11355-reenable-jenkins-notifications;jenkins-jobs:feature/11355-reenable-jenkins-notifications

intrigeri wrote:
> intrigeri wrote:
> > Please ensure the new target version is more realistic than 2.12, after having looked at your 3.0~rc1 and 3.0 tickets. Also, IIRC you mentioned you already have had some WIP locally for a while, that you might want to push to a branch somewhere, so it’s not stored on your hard drive only, and others can help move this forward :)
>
> Ping :)

Right, I’ve just reworked a bit what I had and pushed it. It’s for now completely untested but close to what it will be. If you want to have a first look at the design and code, you’re welcome.

#24 Updated by bertagaz 2017-05-22 12:27:54

• Assignee changed from bertagaz to intrigeri
• % Done changed from 20 to 50
• QA Check set to Ready for QA

intrigeri wrote:
> Very quick initial look:
>
> * I like the design choices, so I’ll only comment about the implementation here. Congrats!

Thanks! Most difficult was to remember how it was done in the several related scripts and repos. :)

> * /usr/local/bin/setup_notifications should have a less generic name

Renamed in 4411e80e7cc40726e708a09af74a6d4427eaab06

> * I’m not happy with the name of command line arguments to setup_notifications; I can certainly improve them myself later on during this process, but I would appreciate if you first sat down, looked at the use cases, thought about how confusing they can be (e.g. disable_build does not disable any “build”) and improved them a bit; hint: the help string for each argument are pretty good and provide good inspiration for the corresponding option name :)

I wasn’t too happy about that too. Tentatively done in 9859ab0093a0e4906fcd452e8fb6225a073811d8.

> * /etc/redmine/apikey should live in /etc/jenkins IMO: it is surely about Redmine, but what it configures is Jenkins stuff, not Redmine

Makes sense. Done in bd7efbb5e9e6636607514807d18ea68b2b9a4ce3

> * I’d vastly prefer if setup_notifications output’ed its results (as a pure function!) instead of reporting them as a side effect by writing to $WORKSPACE/tmp/recipient

Done in 6cb019f677cc7a0924c4046b80be888eaf083a7f

> * I’m confused when looking at the diff of files/jenkins/slaves/isobuilders/collect_build_environment: did you mean NOTIFY_BUILD_TO?

Nop, this script collects the environment variable used during the build to pass and use them in the test job environment. So we don’t care about the build notification recipient here, we just need to pass the test one.

> * Typo in recipent

Corrected that yesterday already,in 7e15570ca653952f17ec17ec37912f025aa5dc3c

> * Please use named arguments for the setup_notifications function instead of positional ones (if Python lets you do that, I don’t remember).

They were already named ones, but in Python they can be used as positional ones too. I’ve still changed that accordingly in 9731bebd335e7010ce3828f81da25f3e746ed9e8

> * ticket_state variable name is confusing: it’s used for something that is not Redmine ticket “Status”.
> * Please extract the checking for RfQA in a predicate function.

Done both in fee1b2c675a0934036130ff42d7f8f1070e70e34.

From local tests I made it works quite well, so I think it can be reviewed now.

#25 Updated by intrigeri 2017-05-23 07:46:42

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

Note: I’ve pushed some (untested) improvements.

> Thanks! Most difficult was to remember how it was done in the several related scripts and repos. :)

Design doc, anyone? (Sorry if I’m beating a dead horse.)

>> * I’m not happy with the name of command line arguments to setup_notifications; I can certainly improve them myself later on during this process, but I would appreciate if you first sat down, looked at the use cases, thought about how confusing they can be (e.g. disable_build does not disable any “build”) and improved them a bit; hint: the help string for each argument are pretty good and provide good inspiration for the corresponding option name :)

> I wasn’t too happy about that too. Tentatively done in 9859ab0093a0e4906fcd452e8fb6225a073811d8.

OK, great! Improved a tiny bit on top.

>> * /etc/redmine/apikey should live in /etc/jenkins IMO: it is surely about Redmine, but what it configures is Jenkins stuff, not Redmine

> Makes sense. Done in bd7efbb5e9e6636607514807d18ea68b2b9a4ce3

Cool. Then please adjust puppet:///modules/tails_secrets_jenkins/jenkins/slaves/isobuilders/redmine/redmine_apikey and require => File['/etc/redmine/apikey'] accordingly (that Puppet code can’t possibly work; please test stuff before sending for QA!).

>> * I’d vastly prefer if setup_notifications output’ed its results (as a pure function!) instead of reporting them as a side effect by writing to $WORKSPACE/tmp/recipient

> Done in 6cb019f677cc7a0924c4046b80be888eaf083a7f

Good. So the name of the setup_notifications function doesn’t reflect what it does anymore. Same for the script: it doesn’t setup anything AFAICT.

>> * I’m confused when looking at the diff of files/jenkins/slaves/isobuilders/collect_build_environment: did you mean NOTIFY_BUILD_TO?

> Nop, this script collects the environment variable used during the build to pass and use them in the test job environment. So we don’t care about the build notification recipient here, we just need to pass the test one.

OK. Please add a comment then :)

> From local tests I made it works quite well, so I think it can be reviewed now.

Cool! How did you test this? In a clone of our Jenkins setup, or anything else? There was a syntax error in the Puppet code so I’m wondering how it can have been tested.

Some more questions/comments:

What’s the requested kill switch? Is it the --disable* args one can pass in macros/builders.yaml?

What happens if TAILS_TICKET == 0 and args.rfqa is true? To me it looks like print(notification_vars) will raise an exception. Same question when args.rfqa is false and TAILS_TICKET == 0. And once you’ve added code to handle the missing cases, IMO the nested if/else thing will have grown out of control and will want a refactoring.

Why does the jenkins user need write access to /etc/jenkins?

#26 Updated by bertagaz 2017-05-24 11:59:48

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

intrigeri wrote:
> Note: I’ve pushed some (untested) improvements.

Thanks! doesn’t seem to break anything.

> > Thanks! Most difficult was to remember how it was done in the several related scripts and repos. :)
>
> Design doc, anyone? (Sorry if I’m beating a dead horse.)

Doesn’t hurt. I’ll do that while doing the documentation for Feature #12409.

> OK, great! Improved a tiny bit on top.

Good one!

> Cool. Then please adjust puppet:///modules/tails_secrets_jenkins/jenkins/slaves/isobuilders/redmine/redmine_apikey and require => File['/etc/redmine/apikey'] accordingly (that Puppet code can’t possibly work; please test stuff before sending for QA!).

Oops, forgot to push the update of the tails_secrets_jenkins repo. Done. Fix the missing one in cbb9224aa082e912358af7557977d73e11dc9f77

> Good. So the name of the setup_notifications function doesn’t reflect what it does anymore. Same for the script: it doesn’t setup anything AFAICT.

Fixed in 6cc5811cb4028d08ebc66ab179d3aa533005f7a1 and b6e15293a33e2df8072544da68745602e0d38431

> > Nop, this script collects the environment variable used during the build to pass and use them in the test job environment. So we don’t care about the build notification recipient here, we just need to pass the test one.
>
> OK. Please add a comment then :)

458c8ef5272719eb3c7c5b6f15589ed42916eb0e

> > From local tests I made it works quite well, so I think it can be reviewed now.
>
> Cool! How did you test this? In a clone of our Jenkins setup, or anything else? There was a syntax error in the Puppet code so I’m wondering how it can have been tested.

I did test the script with a manual test suite. On the puppet side, considering it was just a matter of installing two files, I did not bother testing it and am willing to fix any problem that may arise at the deployment step.

> What’s the requested kill switch? Is it the --disable* args one can pass in macros/builders.yaml?

Yes. I’ll document that in the design doc.

> What happens if TAILS_TICKET == 0 and args.rfqa is true? To me it looks like print(notification_vars) will raise an exception. Same question when args.rfqa is false and TAILS_TICKET == 0. And once you’ve added code to handle the missing cases, IMO the nested if/else thing will have grown out of control and will want a refactoring.

Ooch, my manual test suite did not include testing when ticket was ‘0’. Indeed it would not work. Fixed in 52f0191eb42ba50c69084bce6f3f8b4d2e101f20

> Why does the jenkins user need write access to /etc/jenkins?

Because I’m too permissive? Fixed in 11e204a024367155ecaa41e677829dfca354947b

#28 Updated by intrigeri 2017-05-24 15:53:11

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

The code now looks OK to me, so please go ahead with the deployment whenever you have time to deal with any issues in the following N days. Please notify our fellow contributors before deploying, making it clear that we’ve learned from last year’s experience, and this time we are ready to immediately disable the whole thing as soon as they notice they are ignoring such notifications. Yeepee :)

Then be careful, don’t close this ticket: what’s been done so far is “only” the first iteration and this ticket is about something bigger. I’ll let you decide if using subtasks for the next steps would help you organize your work.

#30 Updated by intrigeri 2017-05-25 05:38:20

• QA Check changed from Dev Needed to Info Needed

intrigeri wrote:
> The code now looks OK to me, so please go ahead with the deployment whenever […]

Oops, I forgot to ask: what’s happening to the notifications for the base branches, with the proposed changes? IMO they should either be kept as-is (i.e. sent to the RMs) for this first iteration, or be sent to the RM + last committer(s?).

#31 Updated by bertagaz 2017-05-25 09:16:16

• QA Check changed from Info Needed to Dev Needed

intrigeri wrote:
> intrigeri wrote:
> > The code now looks OK to me, so please go ahead with the deployment whenever […]
>
> Oops, I forgot to ask: what’s happening to the notifications for the base branches, with the proposed changes? IMO they should either be kept as-is (i.e. sent to the RMs) for this first iteration, or be sent to the RM + last committer(s?).

The proposed change does not modify the current behavior: the script that generates the jobs sets the build and test recipient to tails-rm@ for base branches (as before), while for other branches it sets it to the NOTIFY_*_TO variables, which is then set by the new script at the beginning of the build to the last commit author.

#33 Updated by bertagaz 2017-06-07 08:48:32

• related to Bug #12647: Deal with June 2107 false positive CI notifications added

#34 Updated by bertagaz 2017-06-07 08:50:17

• related to Bug #12648: Deal with July 2017 false positive CI notifications added

#35 Updated by bertagaz 2017-06-07 08:51:35

• related to Bug #12649: Deal with August 2017 false positive CI notifications added

#36 Updated by bertagaz 2017-06-07 08:53:14

• related to Bug #12650: Deal with September 2017 false positive CI notifications added

#37 Updated by bertagaz 2017-06-07 08:59:35

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

intrigeri wrote:
> > The proposed change does not modify the current behavior: […]
>
> Thanks! Sounds perfect to me for this 1st iteration :)

Merged the branches in their respective repos and had to fix some minor bugs. It now works as expected, see https://jenkins.tails.boum.org/view/All/job/build_Tails_ISO_doc-12000-graphical-as-root/24/console for non-RfQA branch, https://jenkins.tails.boum.org/job/build_Tails_ISO_feature-12635-tor-browser-7.0/10/console for a RfQA branch, and also have a look at the base branches.

I’ve also added tickets to track the number of false positives regulary, and sent an email on tails-dev@ to warn people that the notifications are back.

#38 Updated by intrigeri 2017-06-07 13:33:52

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

I had already reviewed the code, and I’ve verified it seems to work fine at first glance. Some remaining work is listed on Feature #11355#note-28 and Feature #11355#note-29 (I suspect subtasks would be better suited to track them though). Next step: ensure there’s a plan to finish this work; the end goal is not clearly defined beyond this first iteration, and I don’t think we should try to define it better now. Let’s wait and see how this first iteration works first.

#42 Updated by intrigeri 2017-06-30 19:12:41

• Priority changed from Elevated to Normal

#43 Updated by bertagaz 2017-07-02 11:37:42

• Priority changed from Normal to Elevated

intrigeri wrote:
> Here’s my first report as a developer:

Thanks!

> in the test suite failure notifications, I can see the name of the scenarios that failed, which is useful info; but apart of that I feel I’m receiving both too much, and too little info, to be in a position to efficiently (and realistically) do something out of such email:

Ok

> * To see the list of failing scenarios, I have to scroll past the last few dozens lines of output, that are most often useless since the failure happened way earlier; this is not very practical. I suggest not emailing this useless, partial output at all.
> * In most cases, the only way I have to triage common false positives (e.g. time sync’ing failures) is to open the test result web page, as the info I need to do so is not in the email I receive. I’m too lazy to do that, e.g. in the last 5 days I’ve received 2 such email/day on average; I suspect other developers won’t do that either, and they’ll essentially start ignoring these email again. if I was sent the output of the failing tests, then I’m pretty sure I would actually do this triaging.

Sadly, the content of the emails sent does not seem easy to customize. If we want to adjust such things, we’ll have to switch to the email-ext plugin.

> Other than that, notifications have been re-enabled 2 weeks ago, and I think it’s time asking contributors what they actually do with them, in order to identify usability issues before they start getting bad habits that’ll be hard to fix later: at least I have already started training myself to vaguely skim through such email without diving into the actual possible problems they’re supposed to make me aware of.

Before doing so, I made some stats to know who has received notifications in June. It appears apart from tails-rm, you’re the only one that had (--rfqa-only!), for the Bug #12686, Bug #12685 and Bug #12684 branch. So I’m not sure such a broad email to the developers is needed. I’ll get anonym’s feedback though.

#45 Updated by bertagaz 2017-07-05 10:39:23

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

intrigeri wrote:
> > I’ll get anonym’s feedback though.
>
> This is needed indeed.

So anonym, what feedbacks do you have regarding Jenkins notifications? Do you read them? Are they useful?

#46 Updated by anonym 2017-07-06 13:46:12

• Assignee changed from anonym to bertagaz
• QA Check deleted (Info Needed)

bertagaz wrote:
> intrigeri wrote:
> > > I’ll get anonym’s feedback though.
> >
> > This is needed indeed.
>
> So anonym, what feedbacks do you have regarding Jenkins notifications? Do you read them? Are they useful?

I’m sorry, but I don’t think my answer is relevant — I was AFK half of this time, and the rest was super stressful for me for various reasons, so I only read the emails I absolutely had to, i.e. I still ignore my folder where the Jenkins notifications are sorted into.

I think I need a “normal” month to evaluate this. I expect September to be such a month. Sorry if this doesn’t help you for what you need now. :/

#48 Updated by intrigeri 2017-07-06 14:11:36

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

(post-September)

#49 Updated by intrigeri 2017-08-28 07:55:57

• blocked by Bug #14459: Some branches fail to build an ISO because we merge their base branch too late added

#51 Updated by anonym 2017-09-28 18:29:26

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

#52 Updated by intrigeri 2017-10-17 16:29:45

• blocked by Bug #14857: Some doc branches fail to build an ISO because the base branch of master is "testing" after a major release added

#53 Updated by bertagaz 2017-10-20 09:11:22

• related to Bug #14871: Deduplicate the ticket_qa_check Redmine query function into the Tails pythonlib added

#54 Updated by bertagaz 2017-10-23 09:52:58

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

Realistically reschedule for 3.4.

#55 Updated by anonym 2018-01-23 19:52:31

• Target version changed from Tails_3.5 to Tails_3.6

#56 Updated by bertagaz 2018-03-14 11:32:07

• Target version changed from Tails_3.6 to Tails_3.7

#57 Updated by intrigeri 2018-04-08 13:09:03

• blocks Feature #13284: Core work: Sysadmin (Adapt our infrastructure) added

#60 Updated by bertagaz 2018-05-10 11:09:03

• Target version changed from Tails_3.7 to Tails_3.8

#61 Updated by intrigeri 2018-06-26 16:27:39

• Target version changed from Tails_3.8 to Tails_3.9

#62 Updated by intrigeri 2018-09-05 16:26:48

• Target version changed from Tails_3.9 to Tails_3.10.1

#63 Updated by intrigeri 2018-10-24 17:03:30

• Target version changed from Tails_3.10.1 to Tails_3.11

#64 Updated by CyrilBrulebois 2018-12-16 13:53:46

• Target version changed from Tails_3.11 to Tails_3.12

#65 Updated by anonym 2019-01-30 11:59:11

• Target version changed from Tails_3.12 to Tails_3.13

#66 Updated by Anonymous 2019-03-14 13:13:58

• Assignee deleted (bertagaz)
• Target version deleted (Tails_3.13)
• Deliverable for deleted (SponsorS_Internal)

We should give up on this and drop the “Deliverable for”. Then, whenever we have a team who’s eager to improve our developers’ workflow with CI tools (and has time for a sustained effort in that direction), we can take a step back and see what we need most. I suspect it won’t be email notifications.

#67 Updated by intrigeri 2019-03-20 15:58:16

• blocked by deleted (Feature #13284: Core work: Sysadmin (Adapt our infrastructure))