Code review comment for lp:~nskaggs/juju-release-tools/add-daily-build

Revision history for this message
Curtis Hovey (sinzui) wrote :

Thank you Nicholas.

The test refactorings were unexpected and challenging to read as they look to like a total rewrite when they are not. In general, a branch should not exceed 400 lines. I would have made the testscenarios change in a separate branch. I can see this is a nice addition. I have but one quibble over a test name in line.

The hosts that use this project will fail their these tests because python-testscenarios is not installed by any of our other deps. This project's deps are unified with juju-ci-tools. It's Makefile will provide the deps for for juju-release-tools. That isn't true any more :(. I accept python-testscenarios. I think we need a new Makefile target that installs: python-testscenarios. The good news is we do not need to make this project work on windows, centos, or os x.

HOWEVER. Something is wrong and I don't see anything wrong in your implementation. Look at
    http://juju-ci.vapour.ws/job/release-juju-create-source-packages-daily/15/artifact/juju-build-trusty-all/juju-core_2.0-beta6-20160503+3921+4bbce805~14.04.dsc
^ I see juju-core_2.0-beta6.orig.tar.gz in there. We don't want that uploaded. I don't see how it exists since I see os.rename. I think the tar file is renamed immediately. We pass the new tarfile name to the step that imports it. I see this confirmed at
    http://juju-ci.vapour.ws/job/release-juju-create-source-packages-daily/ws/juju-build-any-all/
When I look at
     http://juju-ci.vapour.ws/job/release-juju-create-source-packages-daily/ws/juju-build-trusty-all/
I see our new version as the debian tar, but not the orig.

Oh. I think I know. create_source_package_branch() takes version. It "sets" the version of the upstream tarfile being imported...oh wow.

1 we need to pass the new version so that the call to create the pristine-tar returns the right version for the orig.
2. um maybe we didn't need to rename the tarfile because I think the spb's pristine-tar always becomes the .orig.

review: Needs Information (code)

« Back to merge proposal