Merge lp:~fginther/cupstream2distro-config/fix-pbuilder-hooks into lp:cupstream2distro-config

[Francis Ginther]

Use different jenkins parameter names to specify the hooks list for the parent and the child builder jobs. The $hooks value is always inherited from the parent job and as a result, it was not possible to specify unique hooks for the builder jobs using $hooks. The child builder jobs will use $builder_hooks instead.

Use different jenkins parameter names to specify the hooks list for the parent and the child builder jobs. The $hooks value is always inherited from the parent job and as a result, it was not possible to specify unique hooks for the builder jobs using $hooks. The child builder jobs will use $builder_hooks instead.

Some jobs (i.e. compiz and unity) have different hooks for specific pbuilder jobs. The idea was to use these different hooks to skip the coverage build for armhf and get a faster build. However, this is not working in practice because the builder jobs are always inheriting the $hooks value from the parent. The solution is to not use the $hooks parameter for the actual pbuilder hooks value, instead it uses $builder_hooks which contains the value generated at job deployment time and ignores the value of $hooks. This still allows a parent to pass a $hooks value to a non cu2d-config child job.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:332
http://jenkins.qa.ubuntu.com/job/cupstream2distro-config-ci/245/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/cupstream2distro-config-saucy-amd64-ci/43/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/cupstream2distro-config-ci/245/rebuild

review: Needs Fixing (continuous-integration)

[Martin Mrazik (mrazik)]

I guess this is a matter of preference but I always get confused when I see just "True" somewhere:
34 + True)

Can you maybe change it to builder_job=True to improve the readability of the call?

This is not used anywhere (looks like a leftover from copy&paste):
135 + config = {
136 + 'hooks': 'parent-hook',
137 + 'configurations': {
138 + 'raring-amd64': {'node_label': 'pbuilder',
139 + 'hooks': 'child-only-hook'},
140 + 'raring-i386': {'node_label': 'pbuilder'}}}

Not sure if this is the error jenkins complains about but the following fails on my machine:

159 + self.assertEqual(count, 3)

I guess the following should be just +1? :
157 + count += 3

Other than it looks good.

review: Needs Fixing

[lp:~fginther/cupstream2distro-config/fix-pbuilder-hooks]

333. By Francis Ginther on 2013-05-22

Fixed readability issues, removed some unused code and fixed the test_parent_child_hooks test.

[Francis Ginther (fginther)]

> I guess this is a matter of preference but I always get confused when I see
> just "True" somewhere:
> 34 + True)
>
> Can you maybe change it to builder_job=True to improve the readability of the
> call?

No problem.

> This is not used anywhere (looks like a leftover from copy&paste):
> 135 + config = {
> 136 + 'hooks': 'parent-hook',
> 137 + 'configurations': {
> 138 + 'raring-amd64': {'node_label': 'pbuilder',
> 139 + 'hooks': 'child-only-hook'},
> 140 + 'raring-i386': {'node_label': 'pbuilder'}}}
>

Removed.

> Not sure if this is the error jenkins complains about but the following fails
> on my machine:
>
> 159 + self.assertEqual(count, 3)
>
> I guess the following should be just +1? :
> 157 + count += 3
>

Oops, I had modified this to make sure it would catch a failure, then forgot to change it back. Fixed.

> Other than it looks good.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:333
http://jenkins.qa.ubuntu.com/job/cupstream2distro-config-ci/246/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/cupstream2distro-config-saucy-amd64-ci/44

Click here to trigger a rebuild:
http://s-jenkins:8080/job/cupstream2distro-config-ci/246/rebuild

review: Approve (continuous-integration)

[Martin Mrazik (mrazik)]

review: Approve

=== modified file 'c2dconfigutils/cu2dUpdateCi.py'
--- c2dconfigutils/cu2dUpdateCi.py	2013-05-15 15:30:58 +0000
+++ c2dconfigutils/cu2dUpdateCi.py	2013-05-22 13:14:27 +0000
@@ -140,7 +140,7 @@
         ctx['parameter_list'].append(parameter)
 
     def process_project_config(self, project_name, project_config,
-                               job_data):
+                               job_data, builder_job=False):
         """ Generates the template context from a project configuration
 
         :param project_name: the project name from the stack definition
@@ -162,6 +162,14 @@
             else:
                 project_config['hooks'] = stack_ppa_hook
 
+        # Rename hooks parameter for builder jobs
+        if builder_job and 'hooks' in project_config:
+            # The hooks parameter used by the builder job must not have
+            # the same name as the parent job. Otherwise, the builder job
+            # will always inherit the hooks from the parent and builder
+            # specific hooks specified in the stack file will be ignored.
+            project_config['builder_hooks'] = project_config['hooks']
+
         for key in project_config:
             data = project_config[key]
 
@@ -260,7 +268,8 @@
                 else:
                     dict_union(build_config, data)
                     ctx = self.process_project_config(project_name,
-                                                      build_config, job_data)
+                                                      build_config, job_data,
+                                                      builder_job=True)
                     build_name = '-'.join([job_base_name, config_name,
                                            job_type])
                     build_list.append(build_name)

=== modified file 'ci/jenkins-templates/mbs-pbuilder-config.xml.tmpl'
--- ci/jenkins-templates/mbs-pbuilder-config.xml.tmpl	2013-04-26 15:47:24 +0000
+++ ci/jenkins-templates/mbs-pbuilder-config.xml.tmpl	2013-05-22 13:14:27 +0000
@@ -58,7 +58,7 @@
       <command>{{ acquire_hook_script }}</command>
     </hudson.tasks.Shell>
     <com.ubuntu.builder.PBuilderPlugin>
-      <hooks>$hooks</hooks>
+      <hooks>$builder_hooks</hooks>
       <mainBranch>$landing_candidate</mainBranch>
       <packagingBranch>{{ packaging_branch }}</packagingBranch>
       <targetBranch>$target_branch</targetBranch>

=== modified file 'ci/jenkins-templates/pbuilder-config.xml.tmpl'
--- ci/jenkins-templates/pbuilder-config.xml.tmpl	2013-04-26 15:47:24 +0000
+++ ci/jenkins-templates/pbuilder-config.xml.tmpl	2013-05-22 13:14:27 +0000
@@ -48,7 +48,7 @@
       <command>{{ acquire_hook_script }}</command>
     </hudson.tasks.Shell>
     <com.ubuntu.builder.PBuilderPlugin>
-      <hooks>$hooks</hooks>
+      <hooks>$builder_hooks</hooks>
       <mainBranch>$landing_candidate</mainBranch>
       <packagingBranch>{{ packaging_branch }}</packagingBranch>
       <targetBranch>$target_branch</targetBranch>

=== modified file 'tests/test_cu2dUpdateCi.py'
--- tests/test_cu2dUpdateCi.py	2013-05-15 15:30:58 +0000
+++ tests/test_cu2dUpdateCi.py	2013-05-22 13:14:27 +0000
@@ -211,6 +211,40 @@
                                                        job_data)
         self.assertEqual(expected, actual)
 
+    def test_project_config_parent_hooks(self):
+        config = {'hooks': 'parent-hook'}
+        expected = {'target_branch': 'lp:project',
+                    'project_name': 'project',
+                    'hooks': 'parent-hook',
+                    'parameter_list': [
+                        JobParameter('hooks',
+                                     'parent-hook'),
+                        JobParameter('target_branch',
+                                     'lp:project'),
+                        JobParameter('project_name',
+                                     'project')]}
+        actual = self.update_ci.process_project_config('project', config, {})
+        self.assertEqual(expected, actual)
+
+    def test_project_config_builder_hooks(self):
+        config = {'hooks': 'builder-hook'}
+        expected = {'target_branch': 'lp:project',
+                    'project_name': 'project',
+                    'hooks': 'builder-hook',
+                    'builder_hooks': 'builder-hook',
+                    'parameter_list': [
+                        JobParameter('hooks',
+                                     'builder-hook'),
+                        JobParameter('target_branch',
+                                     'lp:project'),
+                        JobParameter('builder_hooks',
+                                     'builder-hook'),
+                        JobParameter('project_name',
+                                     'project')]}
+        actual = self.update_ci.process_project_config('project', config, {},
+                                                       builder_job=True)
+        self.assertEqual(expected, actual)
+
     def test_project_config_aggregate_tests(self):
         config = {'aggregate_tests': 'generic-job'}
         expected = {'target_branch': 'lp:project',
@@ -385,11 +419,13 @@
                     'contact_email': 'address@email',
                     'distributions': 'raring,quantal,precise',
                     'ppa_target': 'ppa:autopilot/ppa',
+                    'hooks': 'parent-hook',
                     'autolanding': {
                         'postbuild_job': 'autopilot-docs-upload',
                         'archive_artifacts': '**/output/*deb',
                         'configurations': {
                             'raring-amd64': {
+                                'hooks': 'builder-hook',
                                 'priority': 10000},
                             'raring-i386': {
                                 'template': 'autopilot-config.xml.tmpl',
@@ -490,8 +526,34 @@
                         child_params = [p for p in
                                         child['ctx']['parameter_list']]
                         for p in parent_params:
+                            # Skip hooks as parent and child may differ
+                            if p.name == 'hooks':
+                                continue
                             self.assertIn(p, child_params)
 
+    def test_parent_child_hooks(self):
+        """Verifies that a child builder job has a unique hook parameter when
+        hooks are specified for the builder job"""
+        actual_name_list = [job['name'] for job in self.job_list]
+        self.assertIn('autopilot-autolanding', actual_name_list)
+        self.assertIn('autopilot-raring-amd64-autolanding', actual_name_list)
+        self.assertIn('autopilot-raring-i386-autolanding', actual_name_list)
+        count = 0
+        for job in self.job_list:
+            if job['name'] == 'autopilot-autolanding':
+                self.assertEqual(job['ctx']['hooks'], 'parent-hook')
+                count += 1
+            if job['name'] == 'autopilot-raring-amd64-autolanding':
+                self.assertEqual(job['ctx']['builder_hooks'], 'builder-hook')
+                self.assertEqual(job['ctx']['hooks'], 'builder-hook')
+                count += 1
+            if job['name'] == 'autopilot-raring-i386-autolanding':
+                self.assertEqual(job['ctx']['builder_hooks'], 'parent-hook')
+                self.assertEqual(job['ctx']['hooks'], 'parent-hook')
+                count += 1
+        # Make sure no assertion groups were missed
+        self.assertEqual(count, 3)
+
     def test_target_project(self):
         job_list = []
         target_project = 'autopilot'