Review/promulgation request for the Redis charm

[Francesco Banconi]

The charm can be found at https://code.launchpad.net/~juju-gui/charms/trusty/redis/trunk

[amir sanjar (asanjar)]

Thanks for your contributions to the Juju Charm community. Adding Redis advanced key-value cache and store to the Charm catalog will give Juju user access to rich data structure features.
However bundletester, requirement for juju automated testing, fails with
ERROR environment is already bootstrapped
make: *** [ftest] Error 1
..

"make test also" failed:
CalledProcessError: Command '['juju-deployer', '-W', '-L', '-c', '/tmp/amulet-juju-deployer-0c6Vhq/deployer-schema.json', '-e', 'local', '-t', '1900', 'local']' returned non-zero exit status 1
-------------------- >> begin captured logging << --------------------
amulet.deployer: DEBUG: Deployer schema
{
  "local": {
    "services": {
      "redis1": {
        "expose": true,
        "num_units": 1,
        "branch": "/home/sanjar/development/charms/trusty/redis",
        "options": {
          "password": "secret",
          "loglevel": "verbose",
          "timeout": 60,
          "databases": 3,
          "logfile": "/tmp/redis.log",
          "port": 4242
        }
      }
    },
    "series": "trusty",
    "relations": []
  }
}
amulet.deployer: DEBUG: juju-deployer -W -L -c /tmp/amulet-juju-deployer-0c6Vhq/deployer-schema.json -e local -t 1900 local
--------------------- >> end captured logging << ---------------------

----------------------------------------------------------------------
Ran 2 tests in 178.059s

FAILED (errors=1)
make: *** [ftest] Error 1

[Cory Johns (johnsca)]

It looks like tests.yaml specifies "bootstrap: true" as well as the Makefile manually calling bootstrap. Perhaps the Makefile can be changed to only conditionally bootstrap (if not already)?

[Francesco Banconi (frankban)]

Running the tests using the bundletester runner should now work.
Please take another look.
Thanks!

[amir sanjar (asanjar)]

Francesca,
How are you setting up your amulet test environment? It is still failing the same way as before on mine.

[Cory Johns (johnsca)]

Francesco,

The tests.yaml change will make it play nicer with bootstrap, but there are still a couple of issues with how the Makefile tries to manage the Juju environment:

* Since the bootstrap is unconditional, if it is run with an already-bootstrapped environment, it will fail due to attempting to re-bootstrap the environment. An easy work-around for this would be to add " || true" to the end of the bootstrap command. That will still show bootstrap error messages in the test output, which could be confusing, but it will at least continue on to run the tests.

* Because you're assuming that you bootstrapped the environment, you're also unconditionally destroying it. However, with the change above, you might be destroying an environment that was created before `make test` was run. It also prevents inspecting the environment in the case of failure, to figure out what went wrong with the charm. I'd lean toward removing the destroy step.

* Probably the biggest issue, to me, is that the environment is hard-coded to local. This will not work in the automated test runner (which hasn't picked up this charm yet for some reason), because it attempts to test against multiple environments such as AWS, and MAAS. It also makes reviewing more difficult, because I like to use charmbox to get isolation so that charms whose tests install apt packages don't pollute my system, and the local provider doesn't work within charmbox. I'd like to see the hard-coded env removed and just have it use the currently active environment.

[Cory Johns (johnsca)]

Another issue I hit, which is more of a bug in bundletester but which could be fixed easily with some simple changes to the Makefile, is that bundletester doesn't work well with .PHONY targets; it just skips them. So, when running bundletester for the first time, before the .venv is set up, it skips the .PHONY setup target and thus doesn't create the .venv. By simply removing the setup target and having the other targets refer directly to $(VENV_ACTIVATE) we can work around the issue in bundletester.

With that and the other suggested changes from my previous comment, I was able to get bundletester to run and everything passed. Charm proof, the icon, the README, metadata.yaml, copyright, and all also look good. The charm code itself looks good, as well. So, other than the aforementioned pain points with getting this working with bundletester (and thus the automated test runner), this charm seems good to me.

[Francesco Banconi (frankban)]

Hi Cory,

thanks for the review!
I fixed the charm following your suggestions:

- The "local" env is no longer used by default when
  JUJU_ENV is not specified. Now the current env is
  used instead.

- The environment is no longer destroyed when ftests
  complete.

- Do not exit if bootstrap returns an error: assume
  the environment to be already bootstrapped.

- Removed the setup phony target to work around the
  bundletester bug.

Could you please take another look?

[Cory Johns (johnsca)]

Francesco,

Looks great, and works perfectly for me against AWS. On local provider, running from within the charmbox container, `juju bootstrap` got a bit confused about the already bootstrapped environment and tried to delete the environment, which screwed up the previous steps. Would you object to adding `--keep-broken` to ensure this works with the automated test runner?

[Francesco Banconi (frankban)]

Hi Cory,

I changed the charm's Makefile so that --keep-broken is now passed to "juju bootstrap".
I hope this fixes the interaction with the test runner.
Could you please take another look?

[Cory Johns (johnsca)]

Thanks, that's perfect. I chafe at the need to add that flag, since it is only necessary for a very specific corner case, but it's unfortunately one that the automated test runner will hit up against.

+1 from me for this charm

[Charles Butler (lazypower)]

One minor nitpick and I'll be happy to promulgate this to the recommended namespace.

The CI system seems to be hinky, and @johnsca had some good recommendations - but according to this bug https://github.com/juju-solutions/bundletester/issues/15 we need to place an entry in the testplan yaml to make it function properly with venv creatoin

virtualenv: false

This will trigger any runs in CI to properly build the virtualenv

Otherwise, looks good to me. Thanks for the contribution Frankban!

[Charles Butler (lazypower)]

Greetings again,

I've gone ahead and made the one line update to the testplan yaml - make sure that you've backported this in your repository @frankban.

You can find your store charm located here: https://code.launchpad.net/~charmers/charms/trusty/redis/trunk along with the issue tracker here: https://code.launchpad.net/~charmers/charms/trusty/redis/trunk

+1 LGTM. Thank you for taking the time to submit this fix for the charm store. We appreciate your work. I've merged this branch and it should be available in the charm store after the next ingestion.

If you have any questions/comments/concerns about the review contact us in #juju on irc.freenode.net or email the mailing list <email address hidden>, or ask a question tagged with "juju" on http://askubuntu.com.

[Francesco Banconi (frankban)]

Hi Charles,
thanks for your patch and for accepting the promulgation request. I ported your bundletester change to the ~juju-gui owned charm.
Should I mark this bug as fix released now?