Tails

#1 Updated by intrigeri 2015-01-09 17:48:45

• blocked by Feature #8654: Have topic branches built using the packages from their base branch's APT repo added

#2 Updated by intrigeri 2015-01-09 17:48:56

• blocked by Feature #8656: Write code that generates a set of Jenkins jobs for all branches we want to automatically build added

#3 Updated by intrigeri 2015-01-09 17:49:12

• blocked by Feature #8482: Survey usual committers about dropping the collective pseudonym for signing commits added

#4 Updated by intrigeri 2015-01-09 17:49:19

• blocks deleted (Feature #8482: Survey usual committers about dropping the collective pseudonym for signing commits)

#5 Updated by intrigeri 2015-05-09 02:01:27

• Target version changed from Tails_1.4 to Tails_1.4.1

Due date: 05/31/2015

Postponing.

#7 Updated by intrigeri 2015-05-15 14:42:24

• blocked by Bug #8912: Nightly built artifacts for disabled jobs are not removed added

#8 Updated by intrigeri 2015-05-15 14:42:43

• blocked by Feature #9350: Jenkins workspaces for disabled jobs are not removed from ISO builders added

#11 Updated by intrigeri 2015-05-28 13:32:41

• Status changed from Confirmed to In Progress
• % Done changed from 0 to 10
• Feature Branch set to puppet-tails:feature/8656-deploy_the_build_all_active_branches_system

Here’s a review. I like it! Here are a few comments:

• In commit 0488e69, you’re passing arguments without quoting them => please use single-quotes around ${tails_repo} and ${jenkins_jobs_repo}
• Why is update_build_tails_iso_jobs.erb still a template? its content seems to be static now.
• puppet-lint will point you to at least one problem, and I’m not going to emulate it :)
• I don’t understand how vcsrepo in tails::pythonlib can possibly create /var/lib/tails_pythonlib, if $repo_user != root. The solution to such problems is generally to manage a dedicated first level directory (/var/lib/tails_pythonlib) with Puppet, give it to $repo_user, and then have $repo_user do whatever it wants in there, e.g. clone stuff in a sub-directory.
• I’d like to be sure that someone has verified that vcsrepo triggers its configured notify on repo updates (aka. git pull), and not only after the initial clone.
• I’m not convinced by the argument that essentially is “let’s include classes from classes with hard-coded parameters, and anyone who wants to override them can include the class in their manifest themselves”, because it encourages production and dev setups to diverge. But I won’t pressure you again to address it => do you mind if I implement myself the classical parameter passthrough from tails::jenkins::master to tails::jenkins::iso_jobs_generator?

#12 Updated by intrigeri 2015-05-29 11:37:14

• blocks #8668 added

#13 Updated by bertagaz 2015-06-19 04:24:51

• Assignee changed from bertagaz to intrigeri
• QA Check set to Ready for QA

intrigeri wrote:
> Here’s a review. I like it! Here are a few comments:
>
> * In commit 0488e69, you’re passing arguments without quoting them => please use single-quotes around ${tails_repo} and ${jenkins_jobs_repo}

Right, didn’t know the variables where still interpreted. Fixed.

> * Why is update_build_tails_iso_jobs.erb still a template? its content seems to be static now.

Ooch, good catch! Fixed.

> * puppet-lint will point you to at least one problem, and I’m not going to emulate it :)

Hmmm, mine raised warnings for undocumented classes. I fixed that, but I don’t see any errors.

> * I don’t understand how vcsrepo in tails::pythonlib can possibly create /var/lib/tails_pythonlib, if $repo_user != root. The solution to such problems is generally to manage a dedicated first level directory (/var/lib/tails_pythonlib) with Puppet, give it to $repo_user, and then have $repo_user do whatever it wants in there, e.g. clone stuff in a sub-directory.

True. I’ve fixed that too I believe.

> * I’d like to be sure that someone has verified that vcsrepo triggers its configured notify on repo updates (aka. git pull), and not only after the initial clone.

It does. Most examples I found online where setup the other way: the target resource was configured to subscribe to the Vcsrepo one. So I turn it out like this.

> * I’m not convinced by the argument that essentially is “let’s include classes from classes with hard-coded parameters, and anyone who wants to override them can include the class in their manifest themselves”, because it encourages production and dev setups to diverge. But I won’t pressure you again to address it => do you mind if I implement myself the classical parameter passthrough from tails::jenkins::master to tails::jenkins::iso_jobs_generator?

Well, feel free to do so if you want. I tend to dislike the trend for classes with huge number of parameters, maybe that’s why I implemented it this way. But I get it might not be so “clean” to workaround it like this.

#14 Updated by intrigeri 2015-06-19 06:43:56

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

(Here also, everything I’m skipping seems good to me.)

>> * I don’t understand how vcsrepo in tails::pythonlib can possibly create /var/lib/tails_pythonlib, if $repo_user != root. The solution to such problems is generally to manage a dedicated first level directory (/var/lib/tails_pythonlib) with Puppet, give it to $repo_user, and then have $repo_user do whatever it wants in there, e.g. clone stuff in a sub-directory.

> True. I’ve fixed that too I believe.

Indeed, now looks good. However, Vcsrepo['/var/lib/tails_pythonlib'], in manifests/jenkins/iso_jobs_generator.pp wasn’t adapted accordingly, right? This raises some doubts wrt. whether this code was actually tested, so I’ll dare pushing untested stuff myself, see below.

>> * I’m not convinced by the argument that essentially is “let’s include classes from classes with hard-coded parameters, and anyone who wants to override them can include the class in their manifest themselves”, because it encourages production and dev setups to diverge. But I won’t pressure you again to address it => do you mind if I implement myself the classical parameter passthrough from
>> tails::jenkins::master to tails::jenkins::iso_jobs_generator?

> Well, feel free to do so if you want. I tend to dislike the trend for classes with huge number of parameters, maybe that’s why I implemented it this way. But I get it might not be so “clean” to workaround it like this.

I’ve done it and it added one parameter to the class, which doesn’t feel too scary. I also did a few more style changes here and there (untested, sorry, I hope I didn’t mess up too much). In particular, see commit 7ebda9a: I didn’t know where the lib was installed so I could not complete what commit 6a08a4c does. I’ll happily complete this work if you tell me where python3 ${clone_basedir}/setup.py install installs stuff.

Please review :)

#15 Updated by bertagaz 2015-06-19 08:36:24

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

intrigeri wrote:
>
> Indeed, now looks good. However, Vcsrepo['/var/lib/tails_pythonlib'], in manifests/jenkins/iso_jobs_generator.pp wasn’t adapted accordingly, right? This raises some doubts wrt. whether this code was actually tested, so I’ll dare pushing untested stuff myself, see below.

Fixed. It hasn’t been tested live I admit. However, I don’t feel like it brings too much risk to break something.

> I’ve done it and it added one parameter to the class, which doesn’t feel too scary. I also did a few more style changes here and there (untested, sorry, I hope I didn’t mess up too much). In particular, see commit 7ebda9a: I didn’t know where the lib was installed so I could not complete what commit 6a08a4c does. I’ll happily complete this work if you tell me where python3 ${clone_basedir}/setup.py install installs stuff.

The pythonlib code is installed in /usr/local/lib/python3.4/dist-packages/tailslib/*.py

> Please review :)

I’m not convinced by dcad15f: the repo was originally setup to the one on lizard supposed to be the origin for the others (including the Immerda’s one), where jenkinsjenkins-master@ already has access and that is automatically pushing to Immerda’s one after some tricky git hooks to tests the pushed jobs and apply them in Jenkins. So I I’m not sure we should use the Immerda one. This change also means we now have to give access to the Immerda repo to jenkinsjenkins-master@. So do we choose to keep this change?

Otherwise, everything looks fine to me.

#16 Updated by intrigeri 2015-06-22 04:41:32

• Assignee changed from intrigeri to bertagaz

>> I’ve done it and it added one parameter to the class, which doesn’t feel too scary. I also did a few more style changes here and there (untested, sorry, I hope I didn’t mess up too much). In particular, see commit 7ebda9a: I didn’t know where the lib was installed so I could not complete what commit 6a08a4c does. I’ll happily complete this work if you tell me where python3 ${clone_basedir}/setup.py install installs stuff.

> The pythonlib code is installed in /usr/local/lib/python3.4/dist-packages/tailslib/*.py

Crap, this is dependent on the Python version. I can’t think of a nice way to clean this up, then. Let’s say the current state is better than nothing, and give up. OK?

>> Please review :)

> I’m not convinced by dcad15f: the repo was originally setup to the one on lizard supposed to be the origin for the others (including the Immerda’s one), where jenkinsjenkins-master@ already has access and that is automatically pushing to Immerda’s one after some tricky git hooks to tests the pushed jobs and apply them in Jenkins.

First of all, the way we use it currently on lizard doesn’t match what we’ve configured in Jenkins: the origin remote in /etc/jenkins_jobs is set to gitolite@puppet-git.lizard:jenkins-jobs.git, while we don’t pass this value to the class, and then if we re-deployed this stuff from scratch, then we would get the default remote, that is tails@git.tails.boum.org:jenkins-jobs.git. I guess stuff was set up by hand and then only partially Puppet-ized. So, it seems that our nodes.pp on lizard should pass the correct $jenkins_jobs_repo parameter to the class. If you agree, please just do it or ask me to do so :)

Once we do that, with dcad15f, the correct (lizard’s) Git repo will be used both for /etc/jenkins_jobs/jobs and for tails::jenkins::iso_jobs_generator. That’s exactly what we want, no?

> So I I’m not sure we should use the Immerda one.

No, indeed we should not use it on lizard. That’s why we should pass a suitable $jenkins_jobs_repo to the class there.

> This change also means we now have to give access to the Immerda repo to jenkinsjenkins-master@.

As explained above, thankfully that’s not the case if I understood the setup correctly :)

Now, IMO one should probably document that $automatic_iso_jobs_generator requires $jenkins_jobs_repo to point to a repo where Jenkins has write access. So, perhaps $jenkins_jobs_repo should simply have no default value, and thus be a mandatory parameter to pass to the class?

#17 Updated by bertagaz 2015-06-22 06:35:35

• QA Check changed from Info Needed to Dev Needed

intrigeri wrote:
>
> > The pythonlib code is installed in /usr/local/lib/python3.4/dist-packages/tailslib/*.py
>
> Crap, this is dependent on the Python version. I can’t think of a nice way to clean this up, then. Let’s say the current state is better than nothing, and give up. OK?

Ack. I’ll open a ticket low prio about that just to keep it documented somewhere and this problem is tracked.

>
> >> Please review :)
>
> > I’m not convinced by dcad15f: the repo was originally setup to the one on lizard supposed to be the origin for the others (including the Immerda’s one), where jenkinsjenkins-master@ already has access and that is automatically pushing to Immerda’s one after some tricky git hooks to tests the pushed jobs and apply them in Jenkins.
>
> First of all, the way we use it currently on lizard doesn’t match what we’ve configured in Jenkins: the origin remote in /etc/jenkins_jobs is set to gitolite@puppet-git.lizard:jenkins-jobs.git, while we don’t pass this value to the class, and then if we re-deployed this stuff from scratch, then we would get the default remote, that is tails@git.tails.boum.org:jenkins-jobs.git. I guess stuff was set up by hand and then only partially Puppet-ized. So, it seems that our nodes.pp on lizard should pass the correct $jenkins_jobs_repo parameter to the class. If you agree, please just do it or ask me to do so :)
>
> Once we do that, with dcad15f, the correct (lizard’s) Git repo will be used both for /etc/jenkins_jobs/jobs and for tails::jenkins::iso_jobs_generator. That’s exactly what we want, no?

Oh, I can’t get how I missed that!

>
> Now, IMO one should probably document that $automatic_iso_jobs_generator requires $jenkins_jobs_repo to point to a repo where Jenkins has write access. So, perhaps $jenkins_jobs_repo should simply have no default value, and thus be a mandatory parameter to pass to the class?

Agree. Will implement that.

#18 Updated by bertagaz 2015-06-22 15:10:08

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

bertagaz wrote:
> intrigeri wrote:
> >
> > Now, IMO one should probably document that $automatic_iso_jobs_generator requires $jenkins_jobs_repo to point to a repo where Jenkins has write access. So, perhaps $jenkins_jobs_repo should simply have no default value, and thus be a mandatory parameter to pass to the class?
>
> Agree. Will implement that.

Done, now is mandatory, documented, with an explicit error if not set.

#19 Updated by intrigeri 2015-06-28 14:01:14

• blocked by Feature #9399: Extend lizard's storage capacity added

#20 Updated by intrigeri 2015-06-28 14:06:50

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

> Done, now is mandatory, documented, with an explicit error if not set.

Cool :) The documentation helps, indeed!

Two comments on these last few commits:

• I don’t understand what the manual implementation of the mandatory parameter adds, compared to default Puppet’s one (that is, when a parameter has no default value, then it’s mandatory). I personally find Puppet’s one clearer (native English speakers, blah :)
• I don’t understand why commit 32aa9a8 removes validation of $jenkins_jobs_repo. Checking that it’s not undef doesn’t guarantee that it’s a string.

=> please explain, or fix.

Also, I think this is blocked by Feature #9399, which won’t be resolved before 1.4.1 is out (in order not to disrupt critical infrastructure), so I guess that this ticket should be postponed to 1.5.

#21 Updated by intrigeri 2015-07-03 01:01:26

• Target version changed from Tails_1.4.1 to Tails_1.5

#22 Updated by bertagaz 2015-07-08 11:56:14

• Assignee changed from bertagaz to intrigeri
• Target version deleted (Tails_1.5)
• % Done changed from 10 to 50
• QA Check changed from Info Needed to Ready for QA

intrigeri wrote:
> Two comments on these last few commits:
>
> * I don’t understand what the manual implementation of the mandatory parameter adds, compared to default Puppet’s one (that is, when a parameter has no default value, then it’s mandatory). I personally find Puppet’s one clearer (native English speakers, blah :)
> * I don’t understand why commit 32aa9a8 removes validation of $jenkins_jobs_repo. Checking that it’s not undef doesn’t guarantee that it’s a string.
>
> => please explain, or fix.

I think I’ve tried to be conservative enough to handle the use case where $jenkins_jobs_repo is set to an empty string, and the way the puppet parser is and will treat this empty string in a boolean if condition. Now that I read the patch, I don’t think it even cover this use case in the end. :)

So well, I’ve settled it back with an empty default value, and validate_string() it again.

If you’re happy with it, might be time to give a live test.

#24 Updated by intrigeri 2015-07-09 01:47:01

• Assignee changed from intrigeri to bertagaz
• % Done changed from 50 to 80
• QA Check deleted (Ready for QA)

I reviewed the latest commits and am happy with them. Congrats!

Reassigning to you for deployment, once Feature #9399 is fixed.

#25 Updated by intrigeri 2015-07-09 01:47:55

• Target version set to Tails_1.5

(Re-adding the milestone, assuming it was dropped by mistake.)

#27 Updated by intrigeri 2015-07-12 02:56:44

• blocks deleted (Feature #9399: Extend lizard's storage capacity)

#29 Updated by bertagaz 2015-07-14 05:28:32

• % Done changed from 80 to 90

intrigeri wrote:
> My understanding is that this isn’t blocked by anything anymore => feel free to go wild.

Had to fix more or less minor issues in the tails::pythonlib and tails::jenkins::iso_jobs_generator manifests as well as the scripts, but also in the Tails pythonlib itself. I’ve opened Bug #9740 for the most concerning issue, but it’s now live and running! You might have noticed changes in the job list already.

Now let see if:

• when a new branch to be pushed in the Tails git repo its corresponding job appears in Jenkins =< 5 minutes after.
• when a branch is merged in a base branch, its job is removed from Jenkins.

#30 Updated by bertagaz 2015-07-15 06:13:54

• Status changed from In Progress to Resolved
• Assignee deleted (bertagaz)
• % Done changed from 90 to 100

bertagaz wrote:
>
> Now let see if:
>
> * when a new branch to be pushed in the Tails git repo its corresponding job appears in Jenkins =< 5 minutes after.
> * when a branch is merged in a base branch, its job is removed from Jenkins.

Pushed a new branch (Feature #8878), and its job appeared in Jenkins, merged another one in devel (Bug #9558), and it was removed from the Jenkins jobs. Works great, so closing this ticket!