Merge lp://staging/~hazmat/pyjuju/maas-with-tags into lp://staging/pyjuju

Reviewers: mp+149586_code.launchpad.net,

Message:
Please take a look.

Description:
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 516.

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

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/7365044/

Affected files:
   A [revision details]
   M juju/providers/maas/maas.py
   M juju/providers/maas/tests/test_launch.py
   M juju/providers/maas/tests/test_maas.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: juju/providers/maas/maas.py
=== modified file 'juju/providers/maas/maas.py'
--- juju/providers/maas/maas.py 2012-11-06 16:51:30 +0000
+++ juju/providers/maas/maas.py 2013-02-20 14:40:46 +0000
@@ -47,17 +47,25 @@
          return match.group("system_id")

-def _int_str(value):
+def _int_str(value, k, c):
      """Convert value to integer then string"""
      return str(int(value))

+def _str(value, k, c):
+ return str(value)
+
+
+def _raw(value, k, c):
+ return c.data.get(k)
+
+
  class MAASClient(MAASOAuthConnection):

      _handled_constraints = (
- ("maas-name", "name", str),
- ("maas-tags", "tags", str),
- ("arch", "arch", str),
+ ("maas-name", "name", _str),
+ ("maas-tags", "tags", _raw),
+ ("arch", "arch", _str),
          ("cpu", "cpu_count", _int_str),
          ("mem", "mem", _int_str),
          )
@@ -140,7 +148,7 @@
              for key_from, key_to, translate in self._handled_constraints:
                  value = constraints.get(key_from, None)
                  if value is not None:
- params[key_to] = translate(value)
+ params[key_to] = translate(value, key_from,
constraints)
          return self.post("api/1.0/nodes/", params)

      def start_node(self, resource_uri, ubuntu_series, user_data):
@@ -154,7 +162,9 @@
          """
          assert isinstance(user_data, str), (
              "User data must be a byte string.")
- params = {"op": "start", "distro_series":
ubuntu_series, "user_data": b64encode(user_data)}
+ params = {"op": "start",
+ "distro_series": ubuntu_series,
+ "user_data": b64encode(user_data)}
          return self.post(resource_uri, params)

      def stop_node(self, resource_uri):

Index: juju/providers/maas/tests/test_launch.py
=== modified file 'juju/providers/maas/tests/test_launch.py'
--- juju/providers/maas/tests/test_launch.py 2012-09-28 11:50:35 +0000
+++ juju/provid...

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#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?

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

https://codereview.appspot.com/7365044/

ux
Thanks for the review.

On Wed, Feb 20, 2013 at 12:50 PM, <email address hidden> 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...

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

https://codereview.appspot.com/7365044/