Merge lp://staging/~nskaggs/juju-release-tools/add-daily-build into lp://staging/juju-release-tools

Proposed by Nicholas Skaggs
Status: Merged
Merged at revision: 299
Proposed branch: lp://staging/~nskaggs/juju-release-tools/add-daily-build
Merge into: lp://staging/juju-release-tools
Diff against target: 561 lines (+311/-132)
2 files modified
build_package.py (+50/-11)
tests/test_build_package.py (+261/-121)
To merge this branch: bzr merge lp://staging/~nskaggs/juju-release-tools/add-daily-build
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+293532@code.staging.launchpad.net

Description of the change

Add date and build for blessed build support

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

I don't think the change is complete and I think this breaks our official releases.

build_package.py is used to make testing an real packages. I think these changes will create packages we intend to release our devel and stable PPAs with dates and hashes in them. The script that extracts agents from packages is sensitive to the binary package names. I doubt agents can be created for official streams because the binary package names will have date and hashes in them.

I think "make test" will fail. juju-release-scripts is a production project that manages releases and the agent archive. We need tests to ensure releases are boring.

I have some suggestions inline.

review: Needs Information (core)
301. By Nicholas Skaggs

add revid too

302. By Nicholas Skaggs

make everything new optional

303. By Nicholas Skaggs

fix ordering and tests

304. By Nicholas Skaggs

add daily tests

Revision history for this message
Nicholas Skaggs (nskaggs) wrote :

I made a new template string. Feel free to comment on things like we really need -ubuntu in there, etc.

I also added tests -- anything else you'd like to see specific to daily?

In addition, I did a test run using this new code with the existing job to ensure I didn't break anything: http://juju-ci.vapour.ws/view/Release, package, publish/job/release-juju-create-source-packages-test/1/

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

Your change looks good, but I believe something is incomplete. Looking at
    http://juju-ci.vapour.ws:8080/job/release-juju-create-source-packages-daily/
I see a orig.tar.gz that is identical to what we have uploaded before:
    juju-core_2.0-beta6.orig.tar.gz

The orig.tar.gz name is identical to the official package we released at
     http://juju-ci.vapour.ws:8080/job/release-juju-create-source-packages/

Lp never forgets a tarfile. A new orig file was not uploaded. Instead, Lp accepted the tarfile because its hash is identical to the one previously uploaded. Anything tarfile that claims to have the same name, btu a different hash will be rejected because it could be a man in the middle attack. When release-juju-create-source-packages-daily creates juju-core_2.0-beta6.orig.tar.gz, the next version (juju-core_2.0-beta7.orig.tar.gz) we will never be able to release 2.0-beta7.

I think a single rule in build_source() will suffice to avoid this problem. Before we create the SourceFile, we get the version from the tarfile. We can rename the tarfile. Maybe we can change these lines

    tarfile_name = os.path.basename(tarfile_path)
    version = tarfile_name.split('_')[-1].replace('.tar.gz', '')

to something like:

    tarfile_name = os.path.basename(tarfile_path)
    version = tarfile_name.split('_')[-1].replace('.tar.gz', '')
    if revid:
        daily_version = {}~{}'.format(version, revid)
        daily_tarfile_name = tarfile_name.replace(version, daily_version)
        tarfile_dir = os.path.dirname(tarfile_path)
        daily_tarfile_path = os.path.join(tarfile_dir, daily_tarfile_name)
        os.rename(tarfile_path, daily_tarfile_path)
        tarfile_path = daily_tarfile_path

^ I typed that really fast. It probably has errors. Maybe we want the orig to have the date and build id too. Maybe this should be extracted to a helper function that is easy to test.

    tarfile_name = os.path.basename(tarfile_path)
    version = tarfile_name.split('_')[-1].replace('.tar.gz', '')
    if revid:
        tarfile_path = rename_daily_tarfile(tarfile_path, version, revid)

review: Needs Fixing (code)
305. By Nicholas Skaggs

add unique name for daily tarball

306. By Nicholas Skaggs

just set names; revamp tests

307. By Nicholas Skaggs

rename

308. By Nicholas Skaggs

flake8

Revision history for this message
Nicholas Skaggs (nskaggs) wrote :

Ok, refactored tests to use scenarios. I stopped myself from going too crazy. I believe you have unique tarballs now too:

http://juju-ci.vapour.ws/job/release-juju-create-source-packages-daily/15/

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)
Revision history for this message
Curtis Hovey (sinzui) wrote :

Hi Nicholas.

After reading this code again. I don't think we need to rename the tarfile. In stead we want to pass a revised version to create_source_package_branch().

1. The call to import-upstream uses version because the upstream tar file is not guaranteed to be debian friendly. The "version" we pass is used to bzr to recreate the orig tarfile as needed.

2. The step to import the tarfile is done in its own sub dir (*.all-all) to keep it isolated from the dirs that make source packages.

3. I think is collides with the changes to create_source_package() which wants to make the source package version. :( Since the version, revid date etc was applied outside of create_source_package() it will make a mistake. Either we continue to pass the original version to this function, or we choose to make this function accept the version passed.

Sorry for advising you

309. By Nicholas Skaggs

fix version for orig tarball

Revision history for this message
Nicholas Skaggs (nskaggs) wrote :

So Curtis, looking this morning the answer was rather simple, heh. I looked at the function and agreed with you. I fixed the test as well and everything has the long version string now. Here's an output for you to grok.

http://juju-ci.vapour.ws/job/release-juju-create-source-packages-daily/21/

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

Thank you very much.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches