460788 - Write a BuildFactory containing all the necessary steps to update the patcher configuration, generate partial mars, and generate update snippets for all past releases.

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Comment 1

[[e-mail] (:bhearsum)]

Attachment: ReleaseUpdatesFactory, v2

Here's a summary of the changes:
* Rename patcherToolsRev to patcherToolsTag (because it's a tag that's applied in 3 different repositories).
* Add some docs - I didn't add them for things like 'version', etc, since they are documented well in patcher-config-bump.pl.
* Checkout build/tools with the patcherToolsTag
* Enable commiting of bumped config file, echo it out before doing so.
* Enable uploading of partial MARs, snippets.
* Enable backupsnip/pushsnip of test snippets

Comment 2

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 343932 [details] [diff] [review]
ReleaseUpdatesFactory, v2

Nice one, r+ with a few changes listed below.

>Index: factory.py
...
>+    def getCandidatesDir(self, product, version, buildNumber):
>+        return os.path.join('/home/ftp/pub/' + product + '/nightly/' + \
>+                            version + '-candidates/build' + str(buildNumber) + \
>+                            '/')

It looks like the os.join.path isn't necessary when concatenating to make the argument ? I think it's safe to assume the ftp server is running Linux and do the concat ourselves. Also, version of 3.1 could accidentally be entered in the release config as a number, so a str(version) might be worth it.

> class ReleaseUpdatesFactory(ReleaseFactory):
...
>+           patcherToolsTag: A tag that is applied to all of:

Nit: s/is/has been/

>+        # If useBetaChannel is False the unnamed snippet type will be
>+        # 'beta' channel snippets. If useBetaChannel is True the unnamed type
>+        # will be 'release' channel snippets

Nit: If it's False, then aus2 will also contain release snippets once we're past a final release.

>         self.addStep(ShellCommand,
>-         command=['cvs', '-d', cvsroot, 'co', '-r', patcherToolsRev,
>+         command=['cvs', '-d', cvsroot, 'co', '-r', patcherToolsTag,
>                   '-d' 'Bootstrap',
>                   'mozilla/tools/release/Bootstrap/Util.pm'],

Driveby nit: this is the only checkout without 'haltonFailure=True'

>+        self.addStep(ShellCommand,
>+         command=['echo', patcherConfigFile]
>+        )

I quite like the diff that Bootstrap does at this point, although I'd prefer it in the unified style. What do you think of adding it here ?

>+        self.addStep(ShellCommand,
>+         command=['cvs', 'commit', '-m',
>+                  'Automated configuration bump: %s, from %s to %s' % \
>+                    (patcherConfig, oldVersion, appVersion)

Enhancement: would be nice if it said "from <oldVersion> to <appVersion> build <buildNumber>"

>         self.addStep(ShellCommand,
>          command=['perl', 'patcher2.pl', '--download',
...
>          haltOnFailure=True

Enhancement: I'd love some description's on all these addSteps, the waterfall is a bit cryptic with just fragments of the command.

>+        self.addStep(ShellCommand,
>+         command=['rsync', '-av',
>+                  '-e', 'ssh -oIdentityFile=~/.ssh/%s' % stageSshKey,
>+                  '--include=*partial.mar', '--exclude=*complete.mar',

Not sure exactly what is on disk at this point, but I'm guessing there are just mar files and the --exclude would be enough. What does the --include get us ?

>+            remoteDir = '%s-%s-%s' % (strftime('%Y%m%d'), productName.title(),
>+                                      appVersion)

Could we move the strftime evaluation outside the loop, in case we start pushing snippets immediately before midnight.

>+            # We only push test channel snippets from automation.
>+            if type == 'test':
>+                self.addStep(ShellCommand,
>+                 command=['ssh', '-l', ausUser, ausHost,
>+                          'cd /opt/aus2/snippets/staging &&' + \
>+                          '~/bin/backupsnip %s' % remoteDir],

The cd isn't required, see the pushd's in the script. Same for pushsnip.

Comment 3

[[e-mail] (:bhearsum)]

Attachment: [checked in] again, address Nick's comments, fix syntax errors

I've addressed all of your comments Nick, and caught a couple of silly syntax errors. I've also added a flag to control the checking in (or not) of the updated patcher config, as discussed in bug 454205.

Comment 4

[[e-mail] (:bhearsum)]

Comment on attachment 344177 [details] [diff] [review]
[checked in] again, address Nick's comments, fix syntax errors

Actually, since we've change useBetaChannel into a boolean in 454205 this is OK.

Comment 5

[[e-mail] (:bhearsum)]

Comment on attachment 344177 [details] [diff] [review]
[checked in] again, address Nick's comments, fix syntax errors

Checking in factory.py;
/cvsroot/mozilla/tools/buildbotcustom/process/factory.py,v  <--  factory.py
new revision: 1.23; previous revision: 1.22
done