cf-charmers team mailing list archive

[Manuel Garcia <mgarciap@xxxxxxxxx>]

Ben, 

Thank you very much for your detailed reply.
Alexanders will start working on this.

On Apr 15, 2014, at 3:48 PM, Benjamin Saller <benjamin.saller@xxxxxxxxxxxxx> wrote:

> Thanks for doing this branch and for using lbox.
> 
> At this time however I'm not comfortable that the testing adds very much
> to our confidence that the charm works.
> 
> Please read over my comments and we can talk about how better to
> proceed.
> 
> 
> 
> https://codereview.appspot.com/88100045/diff/20001/Makefile
> File Makefile (right):
> 
> https://codereview.appspot.com/88100045/diff/20001/Makefile#newcode14
> Makefile:14: sdeb: source
> 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.
> 
> https://codereview.appspot.com/88100045/diff/20001/Makefile#newcode34
> Makefile:34: python setup.py install --user
> 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.
> 
> 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
> I don't think we need/want a README in the bin directory, top level of
> the charm should have the readme
> 
> 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
> Last line very much isn't needed in a charm
> 
> 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)
> 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?
> 
> 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.
> 
> -- 
> Mailing list: https://launchpad.net/~cf-charmers
> Post to     : cf-charmers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~cf-charmers
> More help   : https://help.launchpad.net/ListHelp


-- 
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.