Properly encode maas tags in urls.

Properly encode maas tags in urls.

Hopefully the last in the sordid history of maas tag support. Revno 616 reverted 
attempts to string serialize tags for urls, but which had broken constraint 
comparisions. Those string serialization had been attempt in revno 597 and 598 
to encode maas tags properly for maas client usage. This branch properly 
captures the raw tags string for use by the client, while preserving the 
constraint comparisons restored in revno 616.

https://code.launchpad.net/~hazmat/juju/maas-with-tags/+merge/149586

(do not edit description out of merge proposal)

[hazmat, 2013-02-20 14:46:53]

Please take a look.

[gz, 2013-02-20 17:50:09]

Fix LGTM. The one thing I'm not clear on is if there's still any route for juju
to convert to a set, and stash the constraint somewhere, losing the original
string. As far as I can see not, as there's no deserialisation function.

https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/maas.py
File juju/providers/maas/maas.py (right):

https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/maas.py#new...
juju/providers/maas/maas.py:151: params[key_to] = translate(value, key_from,
constraints)
Should we make the translate functions take (constraint, key_from) instead? Then
rather than doing the get above, it would just be:

    value = translate(constraint, key_from)
    if value is not None:
        params[key_to] = value

Not sure if that's a simplification overall, but would really like to lose some
of the complexity here.

...and thinking about it, makes the translation functions deal with the hasattr
case. Can instead this check for the key without doing the get/conversion step?

https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/tests/test_...
File juju/providers/maas/tests/test_launch.py (right):

https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/tests/test_...
juju/providers/maas/tests/test_launch.py:124: class
FakeWithTags(FakeMAASHTTPConnection):
I'd probably write this test with a reusable Fake class, and avoid having
closure/assertions in it, but this certainly improves our coverage so is a good
step forwards.

https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/tests/test_...
File juju/providers/maas/tests/test_maas.py (right):

https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/tests/test_...
juju/providers/maas/tests/test_maas.py:401: constraints =
constraints.with_series('splendid')
I really liked these tests being actual unit tests that didn't need the whole
provider setup... but I think we're cursed with juju to have everything depend
on everything else.

[hazmat, 2013-02-22 13:10:21]

ux
Thanks for the review.

On Wed, Feb 20, 2013 at 12:50 PM, <martin.packman@canonical.com> wrote:

> Fix LGTM. The one thing I'm not clear on is if there's still any route
> for juju to convert to a set, and stash the constraint somewhere, losing
> the original string. As far as I can see not, as there's no
> deserialisation function.
>

constraints are stored in original form passed post convert/validation on
state objects, and resurrected via subsequent constraint set parse, ie the
original string information is never lost or discarded.


>
>
> https://codereview.appspot.**com/7365044/diff/1/juju/**
>
providers/maas/maas.py<https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/maas.py>
> File juju/providers/maas/maas.py (right):
>
> https://codereview.appspot.**com/7365044/diff/1/juju/**
>
providers/maas/maas.py#**newcode151<https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/maas.py#newcode151>
> juju/providers/maas/maas.py:**151: params[key_to] = translate(value,
> key_from, constraints)
> Should we make the translate functions take (constraint, key_from)
> instead? Then rather than doing the get above, it would just be:
>
>     value = translate(constraint, key_from)
>
>     if value is not None:
>         params[key_to] = value
>
> Not sure if that's a simplification overall, but would really like to
> lose some of the complexity here.

...and thinking about it, makes the translation functions deal with the
> hasattr case. Can instead this check for the key without doing the
> get/conversion step?
>
>
not sure this is really adding anything, feels stylistic. the hasattr case
is handled outside of the value converter with the is None check.


> https://codereview.appspot.**com/7365044/diff/1/juju/**
>
providers/maas/tests/test_**launch.py<https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/tests/test_launch.py>
> File juju/providers/maas/tests/**test_launch.py (right):
>
> https://codereview.appspot.**com/7365044/diff/1/juju/**
>
providers/maas/tests/test_**launch.py#newcode124<https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/tests/test_launch.py#newcode124>
> juju/providers/maas/tests/**test_launch.py:124: class
> FakeWithTags(**FakeMAASHTTPConnection):
> I'd probably write this test with a reusable Fake class, and avoid
> having closure/assertions in it, but this certainly improves our
> coverage so is a good step forwards.
>

given a lack of reuse atm, again feels stylistic.


>
> https://codereview.appspot.**com/7365044/diff/1/juju/**
>
providers/maas/tests/test_**maas.py<https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/tests/test_maas.py>
> File juju/providers/maas/tests/**test_maas.py (right):
>
> https://codereview.appspot.**com/7365044/diff/1/juju/**
>
providers/maas/tests/test_**maas.py#newcode401<https://codereview.appspot.com/7365044/diff/1/juju/providers/maas/tests/test_maas.py#newcode401>
> juju/providers/maas/tests/**test_maas.py:401: constraints =
> constraints.with_series('**splendid')
> I really liked these tests being actual unit tests that didn't need the
> whole provider setup... but I think we're cursed with juju to have
> everything depend on everything else.
>

yes, tests that actually verify do need additional context, else we end up
the back in the poorly tested scenario that caused the numerous issues and
revisions that attempted to address the underlying tags issues.

[hazmat, 2013-02-22 13:12:22]

*** Submitted:

Properly encode maas tags in urls.

Hopefully the last in the sordid history of maas tag support. Revno 616 reverted

attempts to string serialize tags for urls, but which had broken constraint 
comparisions. Those string serialization had been attempt in revno 597 and 598 
to encode maas tags properly for maas client usage. This branch properly 
captures the raw tags string for use by the client, while preserving the 
constraint comparisons restored in revno 616.

R=gz
CC=
https://codereview.appspot.com/7365044