cf-charmers team mailing list archive

[Alex Lomov <lomov.as@xxxxxxxxx>]

Thank you for your response Ben. I will fix it next working day.


On 15 April 2014 22:57, Manuel Garcia <mgarciap@xxxxxxxxx> wrote:

> 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
> You are the owner of lp:~lomov-as/charms/trusty/cf-cloud-controller/tests.
>

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