Add --delete-charms option to reset-db

Add --delete-charms option to reset-db

Get reset-db under test:
 - update makefile targets
 - add test files
 - split body of code out of reset-db into resetdb.py, and convert to object
 - add tests for object

Add --delete-charms option

Add redis to setup.py requires list

Update download-cache setup so that it always does a heavyweight checkout.

https://code.launchpad.net/~abentley/charmworld/optional-charm-deletion/+merge/138587

(do not edit description out of merge proposal)

[abentley-home, 2012-12-06 22:01:45]

Please take a look.

[hazmat, 2012-12-06 22:13:24]

https://codereview.appspot.com/6868073/diff/1/scripts/reset-db
File scripts/reset-db (right):

https://codereview.appspot.com/6868073/diff/1/scripts/reset-db#newcode2
scripts/reset-db:2: from resetdb import ResetDB
if there is value in objectifying and adding tests, which honestly i'm not clear
on given a lack of logic, we should go ahead and move it to the pkg src tree,
instead of creating a parallel src tree. we can just add an entry point to the
module for scriptifying reset-db out of setup.py and drop this file.

https://codereview.appspot.com/6868073/diff/1/scripts/resetdb.py
File scripts/resetdb.py (right):

https://codereview.appspot.com/6868073/diff/1/scripts/resetdb.py#newcode15
scripts/resetdb.py:15: class ResetDB:
py3 it doesn't matter.. but for py2 i always prefer class x(object) for new
style semantics.

[abentley-home, 2012-12-06 22:27:57]

https://codereview.appspot.com/6868073/diff/1/scripts/reset-db
File scripts/reset-db (right):

https://codereview.appspot.com/6868073/diff/1/scripts/reset-db#newcode2
scripts/reset-db:2: from resetdb import ResetDB
On 2012/12/06 22:13:24, hazmat wrote:
> if there is value in objectifying and adding tests, which honestly i'm not
clear
> on given a lack of logic, we should go ahead and move it to the pkg src tree,
> instead of creating a parallel src tree. we can just add an entry point to the
> module for scriptifying reset-db out of setup.py and drop this file.
> 

Untested code is broken code.  Maybe not immediately, but eventually.  In this
case, because the script was untested, Rick broke the code by not ensuring redis
was installed.  So yes, I want to test all the code we use and delete anything
we don't.  I'll go the entry-point route with it if you like, though.

[hazmat, 2012-12-06 22:43:55]

On 2012/12/06 22:27:57, abentley wrote:
> https://codereview.appspot.com/6868073/diff/1/scripts/reset-db
> File scripts/reset-db (right):
> 
> https://codereview.appspot.com/6868073/diff/1/scripts/reset-db#newcode2
> scripts/reset-db:2: from resetdb import ResetDB
> On 2012/12/06 22:13:24, hazmat wrote:
> > if there is value in objectifying and adding tests, which honestly i'm not
> clear
> > on given a lack of logic, we should go ahead and move it to the pkg src
tree,
> > instead of creating a parallel src tree. we can just add an entry point to
the
> > module for scriptifying reset-db out of setup.py and drop this file.
> > 
> 

> Untested code is broken code.  Maybe not immediately, but eventually. 
>  So yes, I want to test all the code we use and delete anything
> we don't. 

its a good statement in principle, but increasing line count, becomes a
maintenance burden as things change. this 'code' was effectively a 10 line shell
script only for developer convenience, not part of the application we're
developing or its deployment. with this branch that's now 174 lines of extra
code that needs maintenance when we're still discussing how we out our tooling
(queues, db structure, etc). Testing to exhaustion is not only exhausting, its
also dentrimental towards agility and refactoring.

We absolutely need to have testing with a goal of getting to 100% coverage of
the *app*, but this wasn't that. Does our buildout usage need unit testing? does
our makefile need unit testing? at some point we need to rely on our tools and
build our app, and be agile about fixing and improving the tools as we go.

> In this
> case, because the script was untested, Rick broke the code by not ensuring
redis
> was installed.

it didn't just break this code, it broke the application, which definitely
should have additional tests to ensure that's not the case. the tests for this
script aren't adding value to what those application scripts would have found.

> I'll go the entry-point route with it if you like, though.

[abentley-home, 2012-12-06 23:19:02]

On 2012/12/06 22:43:55, hazmat wrote:
> >  So yes, I want to test all the code we use and delete anything
> > we don't. 
> 
> its a good statement in principle, but increasing line count, becomes a
> maintenance burden as things change. this 'code' was effectively a 10 line
shell
> script only for developer convenience, not part of the application we're
> developing or its deployment. with this branch that's now 174 lines of extra
> code that needs maintenance when we're still discussing how we out our tooling
> (queues, db structure, etc). Testing to exhaustion is not only exhausting, its
> also dentrimental towards agility and refactoring.

I want the developer tools to work well, so that the project is easy to hack. 
While code that is part of the app or its deployment may be a higher priority to
you, developer-facing code is high-enough priority to me.  After all, that's the
code I have to deal with.

I find that test code usually has an extremely low maintenance cost.

I've never noticed a case where testing was detrimental to agility and
refactoring, and in this case, having tests makes me more comfortable making my
changes.

> We absolutely need to have testing with a goal of getting to 100% coverage of
> the *app*, but this wasn't that. Does our buildout usage need unit testing?
does
> our makefile need unit testing?

Probably not, but integration testing is a good idea.

> > In this
> > case, because the script was untested, Rick broke the code by not ensuring
> redis
> > was installed.
> 
> it didn't just break this code, it broke the application, which definitely
> should have additional tests to ensure that's not the case. the tests for this
> script aren't adding value to what those application scripts would have found.

I was just demonstrating the principle.

[abentley-home, 2012-12-06 23:24:15]

https://codereview.appspot.com/6868073/diff/1/scripts/resetdb.py
File scripts/resetdb.py (right):

https://codereview.appspot.com/6868073/diff/1/scripts/resetdb.py#newcode15
scripts/resetdb.py:15: class ResetDB:
On 2012/12/06 22:13:24, hazmat wrote:
> py3 it doesn't matter.. but for py2 i always prefer class x(object) for new
> style semantics.

Cloud Engineering's standard practice is to use __metaclass__= type.  I think
it's a good practice, because new-style classes are a better default.  It's less
typing, less chance of mistakes, and less adjustment for Cloud Engineering
staff.  So I'd like to use that practice going forward.

[hazmat, 2012-12-07 01:13:47]

On 2012/12/06 23:24:15, abentley wrote:
> https://codereview.appspot.com/6868073/diff/1/scripts/resetdb.py
> File scripts/resetdb.py (right):
> 
> https://codereview.appspot.com/6868073/diff/1/scripts/resetdb.py#newcode15
> scripts/resetdb.py:15: class ResetDB:
> On 2012/12/06 22:13:24, hazmat wrote:
> > py3 it doesn't matter.. but for py2 i always prefer class x(object) for new
> > style semantics.
> 
> Cloud Engineering's standard practice is to use __metaclass__= type.  I think
> it's a good practice, because new-style classes are a better default.  It's
less
> typing, less chance of mistakes, and less adjustment for Cloud Engineering
> staff.  So I'd like to use that practice going forward.

that's fine as well, the point is principle of least surprise when dealing with
classes. ie new style.

[hazmat, 2012-12-07 01:33:46]

On 2012/12/06 23:19:02, abentley wrote:
> On 2012/12/06 22:43:55, hazmat wrote:
> > >  So yes, I want to test all the code we use and delete anything
> > > we don't. 
> > 
> > its a good statement in principle, but increasing line count, becomes a
> > maintenance burden as things change. this 'code' was effectively a 10 line
> shell
> > script only for developer convenience, not part of the application we're
> > developing or its deployment. with this branch that's now 174 lines of extra
> > code that needs maintenance when we're still discussing how we out our
tooling
> > (queues, db structure, etc). Testing to exhaustion is not only exhausting,
its
> > also dentrimental towards agility and refactoring.
> 
> I want the developer tools to work well, so that the project is easy to hack. 

which was the reason i added this tool as well.

> While code that is part of the app or its deployment may be a higher priority
to
> you, developer-facing code is high-enough priority to me.  After all, that's
the
> code I have to deal with.

let's try to avoid personal bias/assumptions and comments directed individually
and focus on the issue at hand. i'm positing two things lines of code == cost to
maintain. and we should pick well the places where expend ourselves on code to
be the most effective. its the nature of computers that at some point we have
trust that they work till we have evidence otherwise (memory, cpu, disk, os,
etc).

so what exactly is the cost here? the cost for the delta under consideration was
in changing/removing 2-5 lines of code or refactoring and adding 100 lines. did
that refactoring or testing make it more robust? easier to maintain? the answer
to both is no. it's a simple linear operation. instead we now have more
complexity in an object model that has no use being subclassed or encapsulated,
we have tests that prove nothing outside of literally what the code was doing
since it had no logic, was a simple boolean operation, and the tests are mocking
their external systems.

> 
> I find that test code usually has an extremely low maintenance cost.

any code has a maintenance burden. canonical is rife with examples where this
burden has impacted the ability of a project to be agile, be it launchpad or
pyjuju, or others. because these things take time.. to develop, to run, and to
refactor. 

> 
> I've never noticed a case where testing was detrimental to agility and
> refactoring, and in this case, having tests makes me more comfortable making
my
> changes.
> 
> > We absolutely need to have testing with a goal of getting to 100% coverage
of
> > the *app*, but this wasn't that. Does our buildout usage need unit testing?
> does
> > our makefile need unit testing?
> 
> Probably not, but integration testing is a good idea.
> 
> > > In this
> > > case, because the script was untested, Rick broke the code by not ensuring
> > redis
> > > was installed.
> > 
> > it didn't just break this code, it broke the application, which definitely
> > should have additional tests to ensure that's not the case. the tests for
this
> > script aren't adding value to what those application scripts would have
found.
> 
> I was just demonstrating the principle.

but it also invalidates why the testing here would have been useful because an
ingest test would have caught it.

[abentley-home, 2012-12-07 04:41:51]

On 2012/12/07 01:33:46, hazmat wrote:
> On 2012/12/06 23:19:02, abentley wrote:
> > While code that is part of the app or its deployment may be a higher
priority
> to
> > you, developer-facing code is high-enough priority to me.  After all, that's
> the
> > code I have to deal with.
> 
> let's try to avoid personal bias/assumptions and comments directed
individually
> and focus on the issue at hand.

I did not intend to make any kind of personal attack, and I'm sorry if it came
off that way.  I think that the facts are not in dispute, and that our
disagreement stems from differences in perspective, so I want to understand your
perspective better.  If I've misstated your perspective, then please let me
know, so I can improve my understanding.

> i'm positing two things lines of code == cost to
> maintain. and we should pick well the places where expend ourselves on code to
> be the most effective.

Your statement implies that all lines of code have the same cost (i.e. if you
hold the cost fixed, you can replace code, but not change the line count), but I
don't know whether that's what you meant.  I think that different kinds of code
have different costs. Since test code improves the maintainability of the code
under test, I think that adding test code can reduce net costs.

I don't completely agree with the idea that "we should pick well the places
where expend ourselves on code to be the most effective".  If we have to pick
all these things on a case-by-case basis, it's hard to see how we can hope to be
effective, given that everyone has a different perspective and will choose
differently.  I would hope that we can agree on some general practices for
guidance.

> so what exactly is the cost here? the cost for the delta under consideration
was
> in changing/removing 2-5 lines of code or refactoring and adding 100 lines.
did
> that refactoring or testing make it more robust? easier to maintain? the
answer
> to both is no.

I think it made it easier to maintain, because it means that people who make
changes have some confidence that if they break it, it will not eat someone's
data.  As you noted, this is a "very dangerous script", and I think extra
assurance that it does the right thing is valuable.

Since I did TDD, the testing does ensure that the option parsing rejects
gibberish options.  I see that as a small improvement in robustness.

Even in such a short script, I have numerous questions.  For example, in actual
use, the script deletes CHARM_INDEX_DIR, though that does not appear to be the
intent.  This is because CHARM_INDEX_DIR is inside CHARM_DIR, and so
shutil.rmtree(config.CHARM_DIR) deletes it.  Is it guaranteed that
CHARM_INDEX_DIR is inside CHARM_DIR?  If so, the script is broken in all cases. 
But if the locations of CHARM_INDEX_DIR and CHARM_DIR are arbitrary, then it's
possible that CHARM_DIR is inside CHARM_INDEX_DIR, and so the new version of the
script cannot reliably avoid deleting CHARM_DIR.

> it's a simple linear operation. instead we now have more
> complexity in an object model that has no use being subclassed or
encapsulated,

As well as supporting polymorphism and encapsulation, I also find objects a
tremendous way to improve the configurability of an operation, which was my
rationale for doing it here.  

> we have tests that prove nothing outside of literally what the code was doing
> since it had no logic

Definitely not the case.  There were definitely if statements in there.

> was a simple boolean operation

I don't know what you mean by this.

> and the tests are mocking
> their external systems.

Right.  That way they demonstrate that the code is doing what we would expect,
without testing the implementations of redis and mongo themselves.  That pushes
them closer to the unit-test end of the unit-test/integration-test spectrum.

> > I find that test code usually has an extremely low maintenance cost.
> 
> any code has a maintenance burden. canonical is rife with examples where this
> burden has impacted the ability of a project to be agile, be it launchpad or
> pyjuju, or others. because these things take time.. to develop, to run, and to
> refactor. 

Great, could you give me some of those examples, so I can understand your
perspective better?  Launchpad examples would be the easiest for me to
understand.

> > > > In this
> > > > case, because the script was untested, Rick broke the code by not
ensuring
> > > redis
> > > > was installed.
> > > 
> > > it didn't just break this code, it broke the application, which definitely
> > > should have additional tests to ensure that's not the case. the tests for
> this
> > > script aren't adding value to what those application scripts would have
> found.
> > 
> > I was just demonstrating the principle.
> 
> but it also invalidates why the testing here would have been useful because an
> ingest test would have caught it.

It wasn't intended to prove that testing here would have been useful, so
contradicting it doesn't invalidate that.

[hazmat, 2012-12-07 16:30:44]

On Thu, Dec 6, 2012 at 11:41 PM, <bentaaron@gmail.com> wrote:

> On 2012/12/07 01:33:46, hazmat wrote:
>
>> On 2012/12/06 23:19:02, abentley wrote:
>> > While code that is part of the app or its deployment may be a higher
>>
> priority
>
>> to
>> > you, developer-facing code is high-enough priority to me.  After
>>
> all, that's
>
>> the
>> > code I have to deal with.
>>
>
>  let's try to avoid personal bias/assumptions and comments directed
>>
> individually
>
>> and focus on the issue at hand.
>>
>
> I did not intend to make any kind of personal attack, and I'm sorry if
> it came off that way.  I think that the facts are not in dispute, and
> that our disagreement stems from differences in perspective, so I want
> to understand your perspective better.  If I've misstated your
> perspective, then please let me know, so I can improve my understanding.
>
>
sounds good.


>
>  i'm positing two things lines of code == cost to
>> maintain. and we should pick well the places where expend ourselves on
>>
> code to
>
>> be the most effective.
>>
>
> Your statement implies that all lines of code have the same cost (i.e.
> if you hold the cost fixed, you can replace code, but not change the
> line count), but I don't know whether that's what you meant.  I think
> that different kinds of code have different costs. Since test code
> improves the maintainability of the code under test, I think that adding
> test code can reduce net cost.


I absolutely agree tests are valuable and helpful to maintenance. But
that's not a universal statement, the tests have to be useful in validating
the code in question. likewise abstractions should serve a purpose. In this
case neither of those statements is true imo.  The primary purpose of this
code is to interact with two external database systems, the tests in
question do nothing to validate that interaction, they simply repeat the
spelling.

I'm making these statement looking out to the future, as the line count
increases over the lifetime of a project, its not really a question of test
vs other code. Its a question of total amount of work required to implement
a change.




> I don't completely agree with the idea that "we should pick well the
> places where expend ourselves on code to be the most effective".  If we
> have to pick all these things on a case-by-case basis, it's hard to see
> how we can hope to be effective, given that everyone has a different
> perspective and will choose differently.  I would hope that we can agree
> on some general practices for guidance.
>
>
I think its important for us to be able to think strategically about the
problem we're solving in the context of the project goals. I've tried to
provide a strategic guideline for that in this merge proposal and hopefully
that becomes clearer further in this comment/reply.


>
>  so what exactly is the cost here? the cost for the delta under
>>
> consideration was
>
>> in changing/removing 2-5 lines of code or refactoring and adding 100
>>
> lines. did
>
>> that refactoring or testing make it more robust? easier to maintain?
>>
> the answer
>
>> to both is no.
>>
>
> I think it made it easier to maintain, because it means that people who
> make changes have some confidence that if they break it, it will not eat
> someone's data.  As you noted, this is a "very dangerous script", and I
> think extra assurance that it does the right thing is valuable.
>

The whole point of the script is to eat someone's data. The best
validation/confidence metric of that is simplicity.


>
> Since I did TDD, the testing does ensure that the option parsing rejects
> gibberish options.  I see that as a small improvement in robustness.

Even in such a short script, I have numerous questions.  For example, in
> actual use, the script deletes CHARM_INDEX_DIR, though that does not
> appear to be the intent.  This is because CHARM_INDEX_DIR is inside
> CHARM_DIR, and so shutil.rmtree(config.CHARM_**DIR) deletes it.  Is it
> guaranteed that CHARM_INDEX_DIR is inside CHARM_DIR?  If so, the script
> is broken in all cases.  But if the locations of CHARM_INDEX_DIR and
> CHARM_DIR are arbitrary, then it's possible that CHARM_DIR is inside
> CHARM_INDEX_DIR, and so the new version of the script cannot reliably
> avoid deleting CHARM_DIR.
>
>
perhaps this discussion would be easier just looking at the 4 line shell
script that i suggest we use to replace this code.

#!/bin/bash
mongo --eval "db.dropDatabase()" juju
redis-cli flushall
rm -Rf ~/var/index




>  it's a simple linear operation. instead we now have more
>> complexity in an object model that has no use being subclassed or
>>
> encapsulated,
>
> As well as supporting polymorphism and encapsulation, I also find
> objects a tremendous way to improve the configurability of an operation,
> which was my rationale for doing it here.
>
>
Which would be useful if this was an operation that needed configurability,
polymorphism, or encapsulation. Simple as possible but no simpler is a good
rule of thumb when evaluating any code. Often object oriented can help
that, in this case not.

>
>  we have tests that prove nothing outside of literally what the code
>>
> was doing
>
>> since it had no logic
>>
>
> Definitely not the case.  There were definitely if statements in there.


>  was a simple boolean operation
>>
>
> I don't know what you mean by this.
>
>
boolean = conditional, the change in question was adding a conditional,
else its a linear operation which hopefully the shell script makes clear.


>
>  and the tests are mocking
>> their external systems.
>>
>
> Right.  That way they demonstrate that the code is doing what we would
> expect, without testing the implementations of redis and mongo
> themselves.  That pushes them closer to the unit-test end of the
> unit-test/integration-test spectrum.
>
>
which means if those implementations had changed in anyway, this test would
not be useful in validating the intent of the script. ie. we'd still have
breakage because the mocking there didn't test anything about that
interaction. Indeed the tests test the remainder, and that's great...


>
>  > I find that test code usually has an extremely low maintenance cost.
>>
>
>  any code has a maintenance burden. canonical is rife with examples
>>
> where this
>
>> burden has impacted the ability of a project to be agile, be it
>>
> launchpad or
>
>> pyjuju, or others. because these things take time.. to develop, to
>>
> run, and to
>
>> refactor.
>>
>
> Great, could you give me some of those examples, so I can understand
> your perspective better?  Launchpad examples would be the easiest for me
> to understand.
>

look strategically at the marketplace of adoption and goals, and where
launchpad is wrt to those for non canonical usage and ask why and how it
could be better. how long does it take to run launchpad tests, does it
impact developer productivity, does it make things like refactoring the
database interaction easier (say switching to sqlalchemy), or make it
easier to change the ui, or improve performance. Sweeping changes to lp,
are made through colossal effort. That said I've never worked on launchpad,
i've looked through the src, but in terms of featuring planning, it seems
even small features can take quite a while. that has a huge impact to
developer velocity and project agility.  Its a great example that lines of
code have a cost, and that test code can impact productivity negatively.

We can't practice the dogma of TDD and OO without understanding there is a
proper time and place for it, and that simplicity is a guiding principle
for understanding when. As in this case where writing 174 lines of code
when a 4 line shell script would do.  In the case of bzr and launchpad they
were primarily developer tools and so rightly the focus should be on making
it easier for developers. In this case we're building an end user app that
is not for developers and our focus should be on them and the features we
build for the app and how we deploy the app.

The invitation is to continue practice TDD and OO because those are great
ways to produce excellent software but to think strategically about the
right place to do it, and if its making the code simpler. In this case, i
think we should use the shell script version i've given here and move on
with working on application features.

cheers,

Kapil