Review for Zulu8 charm: phase 2

[Dmitriy Kozorez]

Phase 2 of Zulu charm review: layered approach. Original charm for Zulu was totally re-worked to fit new approach. Also now it should be able to handle "major-version" option to let user select which major version of JDK to install. JRE is still not supported. Only 64 bit is available.

[Dmitriy Kozorez (dmitriy-kozorez)]

It looks like we have split our work on the new version of charm in to two pieces: technical and legal. Technical is done while legal is still in progress. That's why I ask review for everything except "copyright" and "readme.md" files.
From my point of view we could do it in parallel to get done faster.

[Review Queue (review-queue)]

The results (PASS) are in and available here: http://juju-ci.vapour.ws:8080/job/charm-bundle-test-lxc/1548/

[Review Queue (review-queue)]

The results (PASS) are in and available here: http://juju-ci.vapour.ws:8080/job/charm-bundle-test-aws/1530/

[Kevin W Monroe (kwmonroe)]

Hi Dmitriy,

Thanks for refactoring your Zulu8 charm to use the layered approach! We have a few issues to sort out before I can update this in the recommended section of the charm store, but they shouldn't be too difficult to work through.

First, only deployable charms should be committed to "lp:~azulcharmers/charms/trusty/zulu8/trunk". The ingestion process that makes a charm available from jujucharms.com watches launchpad "charms/<series>/<charm>/trunk" branches. As you may have noticed, the source for layered charms is not deployable on it's own, which means the bits that are currently ingested are not deployable:

https://jujucharms.com/u/azulcharmers/zulu8/trusty/6

Only the resultant charm from 'charm-build' is deployable. To that end, I've made a merge proposal to your branch that includes the full charm resulting from charm-build on your source:

https://code.launchpad.net/~kwmonroe/charms/trusty/zulu8/layered-charm/+merge/279520

You can merge this from your trunk branch with the following:

bzr merge lp:~kwmonroe/charms/trusty/zulu8/layered-charm

Once you commit the merge changes to your trunk branch, the ingestion process should pick up this new deployable charm in about an hour.

[Kevin W Monroe (kwmonroe)]

Whups, hit Post too soon on that last comment... I covered getting the deployable charm on the right branch in comment 4. The next issue concerns the actual layered source. I noticed the following:

1. README.md, copyright, icon.svg, metadata.yaml are all executable. This isn't a big problem, but there's no reason for these to have the +x bit set.

2. README.md still describes relating this charm to any service via the 'juju-info' relation. This needs to reflect the new 'java' relation.

3. The reactive install() function adds the azul apt key, but it doesn't actually add the azul repo, so apt-get install will fail to find the zulu-X package.

4. The reactive check_version() function should not remove or install zulu unless the user has requested a major version change.

5. The reactive uninstall() function should let the user know if Zulu has been removed by setting an appropriate status message.

6. (This one is a biggie) reactive/install needs to call 'reactive_handler_main' at the end. This is how bash charms know how to interact with the reactive framework.

I've addressed all these points in the attached patch, but now we need to pick an appropriate location for your layered source to live. This can be anywhere you like (launchpad, github, etc). The only current restriction is that it must not interfere with the store's ingestion location -- like i mentioned earlier, the store expects a deployable charm to live at lp:~azulcharmers/charms/trusty/zulu8/trunk, so the layered source must live somewhere else.

Let me know once you decide on a location. This can be as simple as a new branch name on launchpad (./zulu8/source versus ./zulu8/trunk), or a directory under your zulu-openjdk github repo, or anything else you'd like. Once you tell me where the layered source will live, I'd be happy to open a pull-request or merge-proposal to make it easier to incorporate fixes for 1-6 above.

Thanks again for the work on making Zulu8 a layered charm! Please let me know if you have any questions/comments/concerns.

[Kevin W Monroe (kwmonroe)]

Moving to 'In Progress' while Dmitriy processes my comments.

[Cory Johns (johnsca)]

I just want to make sure we're absolutely clear. It is imperative that you push your charm source layer to a new location *before* merging Kevin's MP containing the full charm-build result ("built charm"), so that you don't lose the source layer.

The source layer is the part that should be maintained, and the built charm is just a build artifact. Currently, having it in Launchpad is the only way to get it into the store, but work is being done on a "juju publish" command that will remove the need to have the built charm version controlled at all.

Thanks!

[Dmitriy Kozorez (dmitriy-kozorez)]

Kevin, Cory thanks a lot for such a detailed explanation!

So,I've done point by point:
 - adopted Kevin's changes from #5
 - pushed merged source to lp:~azulcharmers/charms/trusty/zulu8/source as it was suggested
 - adopted Kevin changes from #4 and pushed result to lp:~azulcharmers/charms/trusty/zulu8/trunk

Please note, that I still need to get last review on copyright file from our legal guys after all merges. I will change this bug state to "Fix commited" after it.

[Dmitriy Kozorez (dmitriy-kozorez)]

We're done with review of copyright file -- no changes there.
README.md was updated with new contact info.

I think we are ready to go.

[Kevin W Monroe (kwmonroe)]

Hi Dmitriy - the zulu8 layered charm looks great. Thanks for the perseverance to get this into the charm store. We need to add some form of testing here, so I've proposed the following:

https://code.launchpad.net/~kwmonroe/charms/trusty/zulu8/add-tests/+merge/282681

Please let me know if you have any questions/concerns, but this essentially adds the same simple "java -version" test from my openjdk charm to zulu8. It's not robust, but it will at least allow us to verify that zulu8 gets installed and 'java -version' works. Once we get tests in here, it should be smooth sailing to get this into the store. Thanks again for your work on this charm!

[Kevin W Monroe (kwmonroe)]

Moving to In Progress. Please move back to Fix Committed when you're ready for us to take a look at zulu8 tests. Thanks!

[Dmitriy Kozorez (dmitriy-kozorez)]

Hi Kevin,

thanks a lot for your suggestion! Changeset was pushed to lp:~azulcharmers/charms/trusty/zulu8/trunk .
Changing status back to "Fix commited"

[Kevin W Monroe (kwmonroe)]

Hi Dmitriy, apologies for the delay! There have been recent changes to the base and java layers that you leverage, and I wanted to make sure the layered zulu8 charm still worked well with those. I'm happy to say that all looks good and I have updated zulu8 in the charm store (though it could take up to an hour for jujucharms.com to refresh)

Thank you for your efforts to make Zulu8 a layered charm and for your patience during this review. Oh, and congratulations on being the first promulgated layered java charm -- you beat openjdk!

[Kevin W Monroe (kwmonroe)]

It looks like jujucharms.com has refreshed. The updated Zulu8 charm is available here:

https://jujucharms.com/zulu8/

You may deploy this by simply running:

juju deploy zulu8

You may monitor for charm-related bugs here:

https://bugs.launchpad.net/charms/+source/zulu8

Thanks again!

[Dmitriy Kozorez (dmitriy-kozorez)]

Hi Kevin,

thanks for one more fix for the charm!
 Merged with both trunk and source repositories.