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

Proposed by Kapil Thangavelu
Status: Merged
Merged at revision: 618
Proposed branch: lp:~hazmat/pyjuju/maas-with-tags
Merge into: lp:pyjuju
Diff against target: 144 lines (+55/-11)
3 files modified
juju/providers/maas/maas.py (+16/-6)
juju/providers/maas/tests/test_launch.py (+23/-4)
juju/providers/maas/tests/test_maas.py (+16/-1)
To merge this branch: bzr merge lp:~hazmat/pyjuju/maas-with-tags
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+149586@code.launchpad.net

Description of the change

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://codereview.appspot.com/7365044/

To post a comment you must log in.
Revision history for this message
Kapil Thangavelu (hazmat) wrote :
Download full text (6.5 KiB)

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

Read more...

Revision history for this message
Martin Packman (gz) 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.

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/

Revision history for this message
Kapil Thangavelu (hazmat) wrote :
Download full text (3.4 KiB)

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

Read more...

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'juju/providers/maas/maas.py'
2--- juju/providers/maas/maas.py 2012-11-06 16:51:30 +0000
3+++ juju/providers/maas/maas.py 2013-02-20 14:47:52 +0000
4@@ -47,17 +47,25 @@
5 return match.group("system_id")
6
7
8-def _int_str(value):
9+def _int_str(value, k, c):
10 """Convert value to integer then string"""
11 return str(int(value))
12
13
14+def _str(value, k, c):
15+ return str(value)
16+
17+
18+def _raw(value, k, c):
19+ return c.data.get(k)
20+
21+
22 class MAASClient(MAASOAuthConnection):
23
24 _handled_constraints = (
25- ("maas-name", "name", str),
26- ("maas-tags", "tags", str),
27- ("arch", "arch", str),
28+ ("maas-name", "name", _str),
29+ ("maas-tags", "tags", _raw),
30+ ("arch", "arch", _str),
31 ("cpu", "cpu_count", _int_str),
32 ("mem", "mem", _int_str),
33 )
34@@ -140,7 +148,7 @@
35 for key_from, key_to, translate in self._handled_constraints:
36 value = constraints.get(key_from, None)
37 if value is not None:
38- params[key_to] = translate(value)
39+ params[key_to] = translate(value, key_from, constraints)
40 return self.post("api/1.0/nodes/", params)
41
42 def start_node(self, resource_uri, ubuntu_series, user_data):
43@@ -154,7 +162,9 @@
44 """
45 assert isinstance(user_data, str), (
46 "User data must be a byte string.")
47- params = {"op": "start", "distro_series": ubuntu_series, "user_data": b64encode(user_data)}
48+ params = {"op": "start",
49+ "distro_series": ubuntu_series,
50+ "user_data": b64encode(user_data)}
51 return self.post(resource_uri, params)
52
53 def stop_node(self, resource_uri):
54
55=== modified file 'juju/providers/maas/tests/test_launch.py'
56--- juju/providers/maas/tests/test_launch.py 2012-09-28 11:50:35 +0000
57+++ juju/providers/maas/tests/test_launch.py 2013-02-20 14:47:52 +0000
58@@ -3,9 +3,11 @@
59
60 """Tests for juju.providers.maas.launch"""
61
62+import json
63+
64 from StringIO import StringIO
65 from twisted.internet import defer
66-from twisted.internet.defer import inlineCallbacks
67+from twisted.internet.defer import inlineCallbacks, succeed
68
69 from juju.errors import ProviderError
70 from juju.lib.mocker import ANY
71@@ -108,7 +110,7 @@
72 "values")
73
74 @inlineCallbacks
75- def test_launch_with_name_constraint(self):
76+ def test_launch_with_constraints(self):
77 # Try to launch a particular machine by its name using the
78 # "maas-name" constraint.
79
80@@ -116,8 +118,25 @@
81 target_node = NODE_JSON[1]
82 self.assertEqual("moon", target_node["hostname"])
83 machine_id = "foo"
84- provider = self._get_provider(FakeMAASHTTPConnection)
85- constraints = {"maas-name": "moon", "ubuntu-series": "splendid"}
86+
87+ test = self
88+
89+ class FakeWithTags(FakeMAASHTTPConnection):
90+
91+ def acquire_node(self):
92+ test.assertIn("clawed|furry", self.data)
93+ return super(FakeWithTags, self).acquire_node()
94+
95+ def list_tags(self):
96+ return succeed(json.dumps([
97+ {'name': 'furry', 'definition': 'fuzzy', 'comment': ''},
98+ {'name': 'clawed', 'definition': 'curvy', 'comment': ''}
99+ ]))
100+
101+ provider = self._get_provider(FakeWithTags)
102+ cs = yield provider.get_constraint_set()
103+ constraints = cs.parse(["maas-name=moon", "maas-tags=clawed|furry"])
104+ constraints = constraints.with_series('splendid')
105 machine_data = {"machine-id": machine_id, "constraints": constraints}
106 machine_list = yield provider.start_machine(machine_data)
107
108
109=== modified file 'juju/providers/maas/tests/test_maas.py'
110--- juju/providers/maas/tests/test_maas.py 2012-10-27 07:32:50 +0000
111+++ juju/providers/maas/tests/test_maas.py 2013-02-20 14:47:52 +0000
112@@ -11,6 +11,7 @@
113 from twisted.web.error import Error
114
115 from juju.errors import ProviderError
116+from juju.providers.maas import MachineProvider
117 from juju.providers.maas.maas import extract_system_id, MAASClient
118 from juju.providers.maas.tests.testing import (
119 CONFIG, FakeMAASHTTPConnection,
120@@ -383,9 +384,23 @@
121 mem = client.post.params_used.get("mem")
122 self.assertEqual("2048", mem)
123
124+ @inlineCallbacks
125 def test_acquire_node_handles_arbitrary_tag_query(self):
126+ mock_client = self.mocker.patch(MAASClient(CONFIG))
127+ mock_client.list_tags()
128+ self.mocker.result(succeed([
129+ {'name': 'red', 'definition': '', 'comment': ''},
130+ {'name': 'white', 'definition': '', 'comment': ''},
131+ {'name': 'blue', 'definition': '', 'comment': ''}]))
132+ self.mocker.replay()
133+
134+ provider = MachineProvider("mymaas", CONFIG)
135+ provider.maas_client = mock_client
136+ cs = yield provider.get_constraint_set()
137+ constraints = cs.parse(["maas-tags=red&!white|blue"])
138+ constraints = constraints.with_series('splendid')
139+
140 client = self.set_up_client_with_fake()
141- constraints = {"maas-tags": "red&!white|blue"}
142 client.acquire_node(constraints)
143
144 tags = client.post.params_used.get("tags")

Subscribers

People subscribed via source and target branches

to status/vote changes: