cf-charmers team mailing list archive

[Alex Lomov <lomov.as@xxxxxxxxx>]

https://codereview.appspot.com/88100045/diff/20001/Makefile
File Makefile (right):

https://codereview.appspot.com/88100045/diff/20001/Makefile#newcode14
Makefile:14: sdeb: source
I like idea of tests that can be run after you fetch directory and
install dependencies with one command (like `bin/test_setup` or `bundle
install` in ruby). In order to have such thing we can left current idea
of `.whl` files in `deps` folder or can move `deps` folder to separate
repository and fetch it inside `bin/test_setup` script. What do you
think about it?

https://codereview.appspot.com/88100045/diff/20001/Makefile#newcode14
Makefile:14: sdeb: source
On 2014/04/15 18:47:55, benjamin.saller wrote:
> These targets, the comments in all, the deb building, don't make sense
at all in
> the context of a charm.

> Additionally the pattern of replicating the deps to each charm, and
> charm-helpers is becoming unattractive.

> I am torn if we should alter the pattern such that the developer can
see from
> the readme that they should create their own virtual env, once in
their account,
> install the deps and use that single virtual env to run all these
tests.


Done. `sdeb` task is removed in revision #29

https://codereview.appspot.com/88100045/diff/20001/Makefile#newcode34
Makefile:34: python setup.py install --user
On 2014/04/15 18:47:55, benjamin.saller wrote:
> Again, this doesn't make sense, don't blindly copy things. One doesn't
install
> charms in their home directory as a python development project.

Done. `userinstall` task is removed in revision #29.

https://codereview.appspot.com/88100045/diff/20001/bin/README
File bin/README (right):

https://codereview.appspot.com/88100045/diff/20001/bin/README#newcode1
bin/README:1: This directory contains executables for accessing
cf-cloud-controller juju charm functionality
On 2014/04/15 18:47:55, benjamin.saller wrote:
> I don't think we need/want a README in the bin directory, top level of
the charm
> should have the readme

Done. This README removed in revision #29

https://codereview.appspot.com/88100045/diff/20001/bin/test_setup
File bin/test_setup (right):

https://codereview.appspot.com/88100045/diff/20001/bin/test_setup#newcode9
bin/test_setup:9: # python setup.py develop
On 2014/04/15 18:47:55, benjamin.saller wrote:
> Last line very much isn't needed in a charm

Done.

https://codereview.appspot.com/88100045/diff/20001/tests/test_install.py
File tests/test_install.py (right):

https://codereview.appspot.com/88100045/diff/20001/tests/test_install.py#newcode31
tests/test_install.py:31: self.assertTrue(minstall.called)
On 2014/04/15 18:47:55, benjamin.saller wrote:
> This test mocks everything and proves nothing about the code or that
it
> functions as intended.

> The need to use imp.load_source rather than import the module directly
indicates
> there are other structural issues as well.

> Why doesn't the module import directly? It looks like that install's
code is
> contained in two methods that shouldn't be invoked on import. What is
being run
> such that the charm_dir is needed for the import to even work? Can we
fix that
> instead?


1. yes, I needed to put lots of mocks here because cc_db_migrate works
mostly with system command line (`run and install` method). I will
rethink this approach.

2. I use imp.load_source module to access code in `install` hook that is
located in `hooks/install` file and doesn't have `.py` extension.
Obviously we can link `install.py` with `install` file, but I don't like
this solution as it will add additional requirements to our charm
folder. I like solution with `imp.load_source` better. Also we can move
`cc_db_migrate` function to distinct `.py` file and import it, not sure
about it, because we use `cc_db_migrate` function only inside `install`
hook.

3. we can get read of mocking charm_dir before importing `install` hook.
All we need to do is to remove `charm_dir` call into
`install_upstart_scripts` initialization.

https://codereview.appspot.com/88100045/

-- 
https://code.launchpad.net/~lomov-as/charms/trusty/cf-cloud-controller/tests/+merge/215906
Your team Cloud Foundry Charmers is requested to review the proposed merge of lp:~lomov-as/charms/trusty/cf-cloud-controller/tests into lp:~cf-charmers/charms/trusty/cf-cloud-controller/trunk.