[Merge] lp:~sil2100/ubuntu-archive-tools/sru-release-via-britney into lp:ubuntu-archive-tools
Brian Murray
brian at ubuntu.com
Tue Sep 17 15:04:14 UTC 2019
Review: Needs Fixing
I've added some comments in line that need addressing.
Diff comments:
> === modified file 'sru-release'
> --- sru-release 2019-08-27 11:30:27 +0000
> +++ sru-release 2019-09-17 14:05:32 +0000
> @@ -228,81 +233,135 @@
> return versions
>
>
> -def release_package(options, package):
> - '''Release a package.'''
> -
> - pkg_versions_map = get_versions(options, package)
> - if not pkg_versions_map:
> - message = 'ERROR: No such package, ' + package + ', in -proposed, aborting\n'
> - sys.stderr.write(message)
> - sys.exit(1)
> -
> - for pkg, versions in pkg_versions_map.iteritems():
> - print('--- Releasing %s ---' % pkg)
> - print('Proposed: %s' % versions['proposed'])
> - if 'security' in versions:
> - print('Security: %s' % versions['security'])
> - if 'updates' in versions:
> - print('Updates: %s' % versions['updates'])
> - else:
> - print('Release: %s' % versions.get('release'))
> - if options.devel and 'devel' in versions:
> - print('Devel: %s' % versions['devel'])
> -
> - copy = partial(
> - dst_archive.copyPackage, from_archive=src_archive,
> - include_binaries=True, source_name=pkg,
> - version=versions['proposed'], auto_approve=True)
> -
> - if options.devel:
> - if ('devel' not in versions or
> - versions['devel'] in (
> - versions.get('updates', 'notexisting'),
> - versions['release'])):
> - if not options.no_act:
> - copy(to_pocket='Proposed', to_series=devel_series.name)
> - print('Version in %s matches development series, '
> - 'copied to %s-proposed' % (release, devel_series.name))
> - else:
> - print('ERROR: Version in %s does not match development '
> - 'series, not copying' % release)
> -
> - if options.no_act:
> - if options.release:
> - print('Would copy to %s' % release)
> - else:
> - print('Would copy to %s-updates' % release)
> - else:
> - if options.release:
> - # -proposed -> release
> - copy(to_pocket='Release', to_series=release)
> - print('Copied to %s' % release)
> - else:
> - # -proposed -> -updates
> - # only phasing updates for >=raring to start
> - if (release not in ('lucid', 'precise') and
> - package != 'linux' and
> - not package.startswith('linux-') and
> - not options.security):
> - copy(to_pocket='Updates', to_series=release,
> - phased_update_percentage=options.percentage)
> +def release_packages(options, packages):
> + '''Release packages.'''
I think this could be more informative i.e. indicating that packages is a list.
> +
> + pkg_versions_map = {}
> + packages_to_britney = {}
It might help to explain what the data in packages_to_britney looks like e.g. keys are packages, values are a list of versions(?).
> +
> + for package in packages:
> + pkg_versions_map[package] = get_versions(options, package)
> + if not pkg_versions_map[package]:
> + message = ('ERROR: No such package, ' + package + ', in '
> + '-proposed, aborting\n')
> + sys.stderr.write(message)
> + sys.exit(1)
> +
> + for pkg, versions in pkg_versions_map[package].iteritems():
> + print('--- Releasing %s ---' % pkg)
> + print('Proposed: %s' % versions['proposed'])
> + if 'security' in versions:
> + print('Security: %s' % versions['security'])
> + if 'updates' in versions:
> + print('Updates: %s' % versions['updates'])
> + else:
> + print('Release: %s' % versions.get('release'))
> + if options.devel and 'devel' in versions:
> + print('Devel: %s' % versions['devel'])
> +
> + copy = partial(
> + dst_archive.copyPackage, from_archive=src_archive,
> + include_binaries=True, source_name=pkg,
> + version=versions['proposed'], auto_approve=True)
> +
> + if options.devel and not options.britney:
> + if ('devel' not in versions or
> + versions['devel'] in (
> + versions.get('updates', 'notexisting'),
> + versions['release'])):
> + if not options.no_act:
> + copy(to_pocket='Proposed', to_series=devel_series.name)
> + print('Version in %s matches development series, '
> + 'copied to %s-proposed' %
> + (release, devel_series.name))
> else:
> - copy(to_pocket='Updates', to_series=release)
> - print('Copied to %s-updates' % release)
> - if not options.no_bugs:
> - sru_bugs = match_srubugs(options, versions['changesfile'])
> - tag = 'verification-needed-%s' % release
> - for sru_bug in sru_bugs:
> - if tag not in sru_bug.tags:
> - update_sru_bug(sru_bug, pkg)
> + print('ERROR: Version in %s does not match development '
> + 'series, not copying' % release)
>
> - # -proposed -> -security
> - if options.security:
> if options.no_act:
> - print('Would copy to %s-security' % release)
> + if options.release:
> + print('Would copy to %s' % release)
> + else:
> + print('Would copy to %s-updates' % release)
> else:
> - copy(to_pocket='Security', to_series=release)
> - print('Copied to %s-security' % release)
> + if options.release:
> + # -proposed -> release
> + copy(to_pocket='Release', to_series=release)
> + print('Copied to %s' % release)
> + else:
> + # -proposed -> -updates
> + if (package != 'linux' and
> + not package.startswith('linux-') and
> + not options.security):
> + if options.britney:
> + # We can opt in to use britney for the package copy
> + # instead of doing direct pocket copies.
> + packages_to_britney[pkg] = versions['proposed']
> + else:
> + copy(to_pocket='Updates', to_series=release,
> + phased_update_percentage=options.percentage)
> + else:
> + copy(to_pocket='Updates', to_series=release)
> + print('Copied to %s-updates' % release)
> +
> + # -proposed -> -security
> + if options.security:
> + if options.no_act:
> + print('Would copy to %s-security' % release)
> + else:
> + copy(to_pocket='Security', to_series=release)
> + print('Copied to %s-security' % release)
> +
> + # Write hints for britney to copy the selected packages
> + if options.britney and packages_to_britney:
> + release_package_via_britney(options, packages_to_britney)
> + # If everything went well, update the bugs
> + if not options.no_bugs:
> + for pkg_versions in pkg_versions_map.values():
> + for pkg, versions in pkg_versions.iteritems():
> + sru_bugs = match_srubugs(options, versions['changesfile'])
> + tag = 'verification-needed-%s' % release
> + for sru_bug in sru_bugs:
> + if tag not in sru_bug.tags:
> + update_sru_bug(sru_bug, pkg)
> +
> +
> +def release_package_via_britney(options, packages):
> + '''Release selected packages via britney unblock hints.'''
> +
> + hints_path = os.path.join(options.cache, 'hints-ubuntu-%s' % release)
> + hints_file = os.path.join(hints_path, 'sru-release')
> + # Checkout the hints branch
> + if not os.path.exists(hints_path):
> + cmd = ['bzr', 'checkout', '--lightweight',
> + BZR_HINT_BRANCH % release, hints_path]
> + else:
> + cmd = ['bzr', 'update', hints_path]
> + try:
> + print(' '.join(cmd))
I don't think we need this print any more.
> + subprocess.check_call(cmd)
> + except subprocess.CalledProcessError:
> + print("Failed bzr %s for the hints branch at %s" %
> + (cmd[1], hints_path))
This error message should be expanded so that the user knows they should manually ensure that the hints branch gets updated, because there changes may be lost with the 'bzr update' or whatever command we use.
> + raise
Why is there also a raise here? Would it provide more information or can we use sys.exit(1) here.
> + # Create the hint with a timestamp comment
> + timestamp = time.time() # In python2 we can't use datetime.timestamp()
> + unblock_string = '# %s\n' % timestamp
We should include a human readable timestamp too.
> + unblock_string += ''.join(['unblock %s/%s\n' % (pkg, ver)
> + for pkg, ver in packages.iteritems()])
> + unblock_string += '\n'
> + # Update and commit the hint
> + with open(hints_file, 'a+') as f:
> + f.write(unblock_string)
> + cmd = ['bzr', 'commit', '-m', 'sru-release %s %s' %
I think this should be '%s%s' as the join will ensure there is a space for the first package.
> + (release, ' '.join(packages.keys()))]
> + try:
> + subprocess.check_call(cmd)
> + except subprocess.CalledProcessError:
> + print('Failed to bzr commit to the hints file %s' % hints_file)
> + raise
Again is the raise here necessary / appropriate?
> + print('Added hints for promotion in release %s of packages %s' %
> + (release, ' '.join(packages.keys())))
'packages%s' because of the join.
>
>
> if __name__ == '__main__':
> @@ -367,6 +433,14 @@
> sys.stderr.write(e.args[0] + '\n')
> sys.exit(1)
>
> + options.cache = os.path.expanduser(options.cache)
This won't work if a full path is given with --cache but who cares? Maybe there is a smart way to check for the default value though.
> + if not os.path.isdir(options.cache):
> + if os.path.exists(options.cache):
> + print('Cache path %s already exists and is not a directory.'
> + % options.cache)
> + sys.exit(1)
> + os.makedirs(options.cache)
> +
> launchpad = Launchpad.login_with(
> 'ubuntu-archive-tools', options.launchpad_instance, version='devel')
> ubuntu = launchpad.distributions['ubuntu']
--
https://code.launchpad.net/~sil2100/ubuntu-archive-tools/sru-release-via-britney/+merge/372887
Your team Ubuntu Package Archive Administrators is requested to review the proposed merge of lp:~sil2100/ubuntu-archive-tools/sru-release-via-britney into lp:ubuntu-archive-tools.
More information about the ubuntu-archive
mailing list