Merge lp:~frankban/juju-core/bug-1147138-annotations-api into lp:~juju/juju-core/trunk
- bug-1147138-annotations-api
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 980 | ||||
Proposed branch: | lp:~frankban/juju-core/bug-1147138-annotations-api | ||||
Merge into: | lp:~juju/juju-core/trunk | ||||
Diff against target: |
543 lines (+254/-29) 12 files modified
state/api/apiclient.go (+23/-0) state/api/params/params.go (+17/-0) state/apiserver/api_test.go (+123/-4) state/apiserver/apiserver.go (+24/-6) state/machine_test.go (+1/-1) state/service.go (+19/-0) state/service_test.go (+4/-0) state/state.go (+12/-5) state/state_test.go (+17/-11) state/unit_test.go (+1/-1) state/user.go (+12/-0) state/user_test.go (+1/-1) |
||||
To merge this branch: | bzr merge lp:~frankban/juju-core/bug-1147138-annotations-api | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju Engineering | Pending | ||
Review via email: mp+152037@code.launchpad.net |
Commit message
Description of the change
Annotations API functionality
Annotations are now exposed through the API via 'GetAnnotations' and
'SetAnnotation'. These allow a user to set an annotation on an Entity (service,
'unit', 'machine', with environment coming in a future branch), and retrieve
all annotations on an entity. Tests were added around the added functionality.
Note that now the service is an Entity, as suggested by Roger.
Francesco Banconi (frankban) wrote : | # |
Francesco Banconi (frankban) wrote : | # |
Please take a look.
Dimiter Naydenov (dimitern) wrote : | # |
Overall LGTM, just a couple of comments.
https:/
File state/api/
https:/
state/api/
can you skip the keys here? If it's only one {id} will work.
https:/
state/api/
key, Value: value}
similarly - see above
Francesco Banconi (frankban) wrote : | # |
Please take a look.
https:/
File state/api/
https:/
state/api/
On 2013/03/07 14:28:01, dimitern wrote:
> can you skip the keys here? If it's only one {id} will work.
Done.
https:/
state/api/
key, Value: value}
On 2013/03/07 14:28:01, dimitern wrote:
> similarly - see above
Done.
Roger Peppe (rogpeppe) wrote : | # |
LGTM with the below trivial points addressed.
really good stuff, thanks!
https:/
File state/api/
https:/
state/api/
(*Annotations, error) {
i think there's not really a point in having a new type for annotations
here, just to contain a map.
i'd just return the map directly, like the state Annotations method
does.
something like this, perhaps? (note the variable name change,
just to make it obvious what kind of id we're talking about here).
// SetAnnotations returns annotations that have been
// set on the given entity.
func (c *Client) SetAnnotations(
error)
https:/
state/api/
a given entity.
// SetAnnotation sets the annotation with the given key
// on the given entity to the given value.
// Currently annotations are supported on machines,
// services, units and the environment itself.
func (c *Client) SetAnnotation(
?
https:/
File state/api/
https:/
state/api/
s/Id/EntityId/
https:/
state/api/
s/Id/EntityId/
https:/
File state/apiserver
https:/
state/apiserver
the returned function should reset the annotation to its original value
- all these op* functions should leave the state unchanged.
https:/
state/apiserver
s/Annotations/Check annotations/ ?
https:/
state/apiserver
check error
https:/
state/apiserver
s/Annotations/Check annotations/ ?
https:/
state/apiserver
check error
https:/
state/apiserver
check error
https:/
File state/apiserver
https:/
state/apiserver
GetA...
Francesco Banconi (frankban) wrote : | # |
*** Submitted:
Annotations API functionality
Annotations are now exposed through the API via 'GetAnnotations' and
'SetAnnotation'. These allow a user to set an annotation on an Entity
(service,
'unit', 'machine', with environment coming in a future branch), and
retrieve
all annotations on an entity. Tests were added around the added
functionality.
Note that now the service is an Entity, as suggested by Roger.
R=dimitern, rog
CC=
https:/
https:/
File state/api/
https:/
state/api/
(*Annotations, error) {
On 2013/03/08 20:52:37, rog wrote:
> i think there's not really a point in having a new type for
annotations here,
> just to contain a map.
> i'd just return the map directly, like the state Annotations method
does.
> something like this, perhaps? (note the variable name change,
> just to make it obvious what kind of id we're talking about here).
> // SetAnnotations returns annotations that have been
> // set on the given entity.
> func (c *Client) SetAnnotations(
error)
Done.
https:/
state/api/
a given entity.
On 2013/03/08 20:52:37, rog wrote:
> // SetAnnotation sets the annotation with the given key
> // on the given entity to the given value.
> // Currently annotations are supported on machines,
> // services, units and the environment itself.
> func (c *Client) SetAnnotation(
> ?
Done.
https:/
File state/api/
https:/
state/api/
On 2013/03/08 20:52:37, rog wrote:
> s/Id/EntityId/
Done.
https:/
state/api/
On 2013/03/08 20:52:37, rog wrote:
> s/Id/EntityId/
Done.
https:/
File state/apiserver
https:/
state/apiserver
On 2013/03/08 20:52:37, rog wrote:
> the returned function should reset the annotation to its original
value - all
> these op* functions should leave the state unchanged.
Done.
https:/
state/apiserver
On 2013/03/08 20:52:37, rog wrote:
> s/Annotations/Check annotations/ ?
Done.
https:/
state/apiserver
On 2013/03/08 20:52:37, rog wrote:
> check error
Done.
Francesco Banconi (frankban) wrote : | # |
Hi Dimiter and Roger,
thanks for the reviews.
Preview Diff
1 | === modified file 'state/api/apiclient.go' | |||
2 | --- state/api/apiclient.go 2013-03-08 20:52:12 +0000 | |||
3 | +++ state/api/apiclient.go 2013-03-08 21:34:19 +0000 | |||
4 | @@ -179,6 +179,29 @@ | |||
5 | 179 | return newAllWatcher(c, &info.AllWatcherId), nil | 179 | return newAllWatcher(c, &info.AllWatcherId), nil |
6 | 180 | } | 180 | } |
7 | 181 | 181 | ||
8 | 182 | // GetAnnotations returns annotations that have been set on the given entity. | ||
9 | 183 | func (c *Client) GetAnnotations(entityId string) (map[string]string, error) { | ||
10 | 184 | args := params.GetAnnotations{entityId} | ||
11 | 185 | ann := new(params.GetAnnotationsResults) | ||
12 | 186 | err := c.st.client.Call("Client", "", "GetAnnotations", args, ann) | ||
13 | 187 | if err != nil { | ||
14 | 188 | return nil, clientError(err) | ||
15 | 189 | } | ||
16 | 190 | return ann.Annotations, nil | ||
17 | 191 | } | ||
18 | 192 | |||
19 | 193 | // SetAnnotation sets the annotation with the given key on the given entity to | ||
20 | 194 | // the given value. Currently annotations are supported on machines, services, | ||
21 | 195 | // units and the environment itself. | ||
22 | 196 | func (c *Client) SetAnnotation(entityId, key, value string) error { | ||
23 | 197 | args := params.SetAnnotation{entityId, key, value} | ||
24 | 198 | err := c.st.client.Call("Client", "", "SetAnnotation", args, nil) | ||
25 | 199 | if err != nil { | ||
26 | 200 | return clientError(err) | ||
27 | 201 | } | ||
28 | 202 | return nil | ||
29 | 203 | } | ||
30 | 204 | |||
31 | 182 | // Machine returns a reference to the machine with the given id. | 205 | // Machine returns a reference to the machine with the given id. |
32 | 183 | func (st *State) Machine(id string) (*Machine, error) { | 206 | func (st *State) Machine(id string) (*Machine, error) { |
33 | 184 | m := &Machine{ | 207 | m := &Machine{ |
34 | 185 | 208 | ||
35 | === modified file 'state/api/params/params.go' | |||
36 | --- state/api/params/params.go 2013-03-08 20:36:42 +0000 | |||
37 | +++ state/api/params/params.go 2013-03-08 21:34:19 +0000 | |||
38 | @@ -93,6 +93,23 @@ | |||
39 | 93 | // future. | 93 | // future. |
40 | 94 | } | 94 | } |
41 | 95 | 95 | ||
42 | 96 | // GetAnnotationsResults holds annotations associated with an entity. | ||
43 | 97 | type GetAnnotationsResults struct { | ||
44 | 98 | Annotations map[string]string | ||
45 | 99 | } | ||
46 | 100 | |||
47 | 101 | // GetAnnotations stores parameters for making the GetAnnotations call. | ||
48 | 102 | type GetAnnotations struct { | ||
49 | 103 | EntityId string | ||
50 | 104 | } | ||
51 | 105 | |||
52 | 106 | // SetAnnotation stores parameters for making the SetAnnotation call. | ||
53 | 107 | type SetAnnotation struct { | ||
54 | 108 | EntityId string | ||
55 | 109 | Key string | ||
56 | 110 | Value string | ||
57 | 111 | } | ||
58 | 112 | |||
59 | 96 | // Delta holds details of a change to the environment. | 113 | // Delta holds details of a change to the environment. |
60 | 97 | type Delta struct { | 114 | type Delta struct { |
61 | 98 | // If Removed is true, the entity has been removed; | 115 | // If Removed is true, the entity has been removed; |
62 | 99 | 116 | ||
63 | === modified file 'state/apiserver/api_test.go' | |||
64 | --- state/apiserver/api_test.go 2013-03-08 20:36:42 +0000 | |||
65 | +++ state/apiserver/api_test.go 2013-03-08 21:34:19 +0000 | |||
66 | @@ -85,6 +85,14 @@ | |||
67 | 85 | op: opClientServiceUnexpose, | 85 | op: opClientServiceUnexpose, |
68 | 86 | allow: []string{"user-admin", "user-other"}, | 86 | allow: []string{"user-admin", "user-other"}, |
69 | 87 | }, { | 87 | }, { |
70 | 88 | about: "Client.GetAnnotations", | ||
71 | 89 | op: opClientGetAnnotations, | ||
72 | 90 | allow: []string{"user-admin", "user-other"}, | ||
73 | 91 | }, { | ||
74 | 92 | about: "Client.SetAnnotation", | ||
75 | 93 | op: opClientSetAnnotation, | ||
76 | 94 | allow: []string{"user-admin", "user-other"}, | ||
77 | 95 | }, { | ||
78 | 88 | about: "Client.ServiceAddUnits", | 96 | about: "Client.ServiceAddUnits", |
79 | 89 | op: opClientServiceAddUnits, | 97 | op: opClientServiceAddUnits, |
80 | 90 | allow: []string{"user-admin", "user-other"}, | 98 | allow: []string{"user-admin", "user-other"}, |
81 | @@ -282,6 +290,27 @@ | |||
82 | 282 | return func() {}, nil | 290 | return func() {}, nil |
83 | 283 | } | 291 | } |
84 | 284 | 292 | ||
85 | 293 | func opClientGetAnnotations(c *C, st *api.State) (func(), error) { | ||
86 | 294 | ann, err := st.Client().GetAnnotations("service-wordpress") | ||
87 | 295 | if err != nil { | ||
88 | 296 | return func() {}, err | ||
89 | 297 | } | ||
90 | 298 | c.Assert(err, IsNil) | ||
91 | 299 | c.Assert(ann, DeepEquals, make(map[string]string)) | ||
92 | 300 | return func() {}, nil | ||
93 | 301 | } | ||
94 | 302 | |||
95 | 303 | func opClientSetAnnotation(c *C, st *api.State) (func(), error) { | ||
96 | 304 | err := st.Client().SetAnnotation("service-wordpress", "key", "value") | ||
97 | 305 | if err != nil { | ||
98 | 306 | return func() {}, err | ||
99 | 307 | } | ||
100 | 308 | c.Assert(err, IsNil) | ||
101 | 309 | return func() { | ||
102 | 310 | st.Client().SetAnnotation("service-wordpress", "key", "") | ||
103 | 311 | }, nil | ||
104 | 312 | } | ||
105 | 313 | |||
106 | 285 | func opClientServiceAddUnits(c *C, st *api.State) (func(), error) { | 314 | func opClientServiceAddUnits(c *C, st *api.State) (func(), error) { |
107 | 286 | // This test only checks that the call is made without error, ensuring the | 315 | // This test only checks that the call is made without error, ensuring the |
108 | 287 | // signatures match. | 316 | // signatures match. |
109 | @@ -353,7 +382,7 @@ | |||
110 | 353 | // environment manager (bootstrap machine), so is | 382 | // environment manager (bootstrap machine), so is |
111 | 354 | // hopefully easier to remember as such. | 383 | // hopefully easier to remember as such. |
112 | 355 | func (s *suite) setUpScenario(c *C) (entities []string) { | 384 | func (s *suite) setUpScenario(c *C) (entities []string) { |
114 | 356 | add := func(e state.AuthEntity) { | 385 | add := func(e state.Entity) { |
115 | 357 | entities = append(entities, e.EntityName()) | 386 | entities = append(entities, e.EntityName()) |
116 | 358 | } | 387 | } |
117 | 359 | u, err := s.State.User("admin") | 388 | u, err := s.State.User("admin") |
118 | @@ -427,9 +456,10 @@ | |||
119 | 427 | return | 456 | return |
120 | 428 | } | 457 | } |
121 | 429 | 458 | ||
125 | 430 | // AuthEntity is the same as state.AuthEntity but | 459 | // AuthEntity is the same as state.Entity but |
126 | 431 | // without PasswordValid, which is implemented | 460 | // without PasswordValid and annotations handling |
127 | 432 | // by state entities but not by api entities. | 461 | // which are implemented by state entities but not |
128 | 462 | // by api entities. | ||
129 | 433 | type AuthEntity interface { | 463 | type AuthEntity interface { |
130 | 434 | EntityName() string | 464 | EntityName() string |
131 | 435 | SetPassword(pass string) error | 465 | SetPassword(pass string) error |
132 | @@ -579,6 +609,95 @@ | |||
133 | 579 | c.Assert(info.ProviderType, Equals, conf.Type()) | 609 | c.Assert(info.ProviderType, Equals, conf.Type()) |
134 | 580 | } | 610 | } |
135 | 581 | 611 | ||
136 | 612 | var clientAnnotationsTests = []struct { | ||
137 | 613 | about string | ||
138 | 614 | initial map[string]string | ||
139 | 615 | input map[string]string | ||
140 | 616 | expected map[string]string | ||
141 | 617 | err string | ||
142 | 618 | }{ | ||
143 | 619 | { | ||
144 | 620 | about: "test setting an annotation", | ||
145 | 621 | input: map[string]string{"mykey": "myvalue"}, | ||
146 | 622 | expected: map[string]string{"mykey": "myvalue"}, | ||
147 | 623 | }, | ||
148 | 624 | { | ||
149 | 625 | about: "test setting multiple annotations", | ||
150 | 626 | input: map[string]string{"key1": "value1", "key2": "value2"}, | ||
151 | 627 | expected: map[string]string{"key1": "value1", "key2": "value2"}, | ||
152 | 628 | }, | ||
153 | 629 | { | ||
154 | 630 | about: "test overriding annotations", | ||
155 | 631 | initial: map[string]string{"mykey": "myvalue"}, | ||
156 | 632 | input: map[string]string{"mykey": "another-value"}, | ||
157 | 633 | expected: map[string]string{"mykey": "another-value"}, | ||
158 | 634 | }, | ||
159 | 635 | { | ||
160 | 636 | about: "test setting an invalid annotation", | ||
161 | 637 | input: map[string]string{"invalid.key": "myvalue"}, | ||
162 | 638 | err: `invalid key "invalid.key"`, | ||
163 | 639 | }, | ||
164 | 640 | } | ||
165 | 641 | |||
166 | 642 | func (s *suite) TestClientAnnotations(c *C) { | ||
167 | 643 | // Set up entities. | ||
168 | 644 | service, err := s.State.AddService("dummy", s.AddTestingCharm(c, "dummy")) | ||
169 | 645 | c.Assert(err, IsNil) | ||
170 | 646 | unit, err := service.AddUnit() | ||
171 | 647 | c.Assert(err, IsNil) | ||
172 | 648 | machine, err := s.State.AddMachine("series", state.JobHostUnits) | ||
173 | 649 | c.Assert(err, IsNil) | ||
174 | 650 | entities := []state.Entity{service, unit, machine} | ||
175 | 651 | for i, t := range clientAnnotationsTests { | ||
176 | 652 | loop: | ||
177 | 653 | for _, entity := range entities { | ||
178 | 654 | id := entity.EntityName() | ||
179 | 655 | c.Logf("test %d. %s. entity %s", i, t.about, id) | ||
180 | 656 | // Set initial entity annotations. | ||
181 | 657 | for key, value := range t.initial { | ||
182 | 658 | err := entity.SetAnnotation(key, value) | ||
183 | 659 | c.Assert(err, IsNil) | ||
184 | 660 | } | ||
185 | 661 | // Add annotations using the API call. | ||
186 | 662 | for key, value := range t.input { | ||
187 | 663 | err := s.APIState.Client().SetAnnotation(id, key, value) | ||
188 | 664 | if t.err != "" { | ||
189 | 665 | c.Assert(err, ErrorMatches, t.err) | ||
190 | 666 | continue loop | ||
191 | 667 | } | ||
192 | 668 | c.Assert(err, IsNil) | ||
193 | 669 | } | ||
194 | 670 | // Check annotations are correctly set. | ||
195 | 671 | err := entity.Refresh() | ||
196 | 672 | c.Assert(err, IsNil) | ||
197 | 673 | c.Assert(entity.Annotations(), DeepEquals, t.expected) | ||
198 | 674 | // Retrieve annotations using the API call. | ||
199 | 675 | ann, err := s.APIState.Client().GetAnnotations(id) | ||
200 | 676 | c.Assert(err, IsNil) | ||
201 | 677 | // Check annotations are correctly returned. | ||
202 | 678 | err = entity.Refresh() | ||
203 | 679 | c.Assert(err, IsNil) | ||
204 | 680 | c.Assert(ann, DeepEquals, entity.Annotations()) | ||
205 | 681 | // Clean up annotations on the current entity. | ||
206 | 682 | for key := range entity.Annotations() { | ||
207 | 683 | err = entity.SetAnnotation(key, "") | ||
208 | 684 | c.Assert(err, IsNil) | ||
209 | 685 | } | ||
210 | 686 | } | ||
211 | 687 | } | ||
212 | 688 | } | ||
213 | 689 | |||
214 | 690 | func (s *suite) TestClientAnnotationsBadEntity(c *C) { | ||
215 | 691 | bad := []string{"", "machine", "-foo", "foo-", "---", "machine-jim", "unit-123", "unit-foo", "service-", "service-foo/bar"} | ||
216 | 692 | expected := `invalid entity name ".*"` | ||
217 | 693 | for _, id := range bad { | ||
218 | 694 | err := s.APIState.Client().SetAnnotation(id, "mykey", "myvalue") | ||
219 | 695 | c.Assert(err, ErrorMatches, expected) | ||
220 | 696 | _, err = s.APIState.Client().GetAnnotations(id) | ||
221 | 697 | c.Assert(err, ErrorMatches, expected) | ||
222 | 698 | } | ||
223 | 699 | } | ||
224 | 700 | |||
225 | 582 | func (s *suite) TestMachineLogin(c *C) { | 701 | func (s *suite) TestMachineLogin(c *C) { |
226 | 583 | stm, err := s.State.AddMachine("series", state.JobHostUnits) | 702 | stm, err := s.State.AddMachine("series", state.JobHostUnits) |
227 | 584 | c.Assert(err, IsNil) | 703 | c.Assert(err, IsNil) |
228 | 585 | 704 | ||
229 | === modified file 'state/apiserver/apiserver.go' | |||
230 | --- state/apiserver/apiserver.go 2013-03-08 20:36:42 +0000 | |||
231 | +++ state/apiserver/apiserver.go 2013-03-08 21:34:19 +0000 | |||
232 | @@ -346,6 +346,24 @@ | |||
233 | 346 | return info, nil | 346 | return info, nil |
234 | 347 | } | 347 | } |
235 | 348 | 348 | ||
236 | 349 | // GetAnnotations returns annotations about a given entity. | ||
237 | 350 | func (c *srvClient) GetAnnotations(args params.GetAnnotations) (params.GetAnnotationsResults, error) { | ||
238 | 351 | entity, err := c.root.srv.state.Entity(args.EntityId) | ||
239 | 352 | if err != nil { | ||
240 | 353 | return params.GetAnnotationsResults{}, err | ||
241 | 354 | } | ||
242 | 355 | return params.GetAnnotationsResults{Annotations: entity.Annotations()}, nil | ||
243 | 356 | } | ||
244 | 357 | |||
245 | 358 | // SetAnnotation stores an annotation about a given entity. | ||
246 | 359 | func (c *srvClient) SetAnnotation(args params.SetAnnotation) error { | ||
247 | 360 | entity, err := c.root.srv.state.Entity(args.EntityId) | ||
248 | 361 | if err != nil { | ||
249 | 362 | return err | ||
250 | 363 | } | ||
251 | 364 | return entity.SetAnnotation(args.Key, args.Value) | ||
252 | 365 | } | ||
253 | 366 | |||
254 | 349 | // Login logs in with the provided credentials. | 367 | // Login logs in with the provided credentials. |
255 | 350 | // All subsequent requests on the connection will | 368 | // All subsequent requests on the connection will |
256 | 351 | // act as the authenticated user. | 369 | // act as the authenticated user. |
257 | @@ -370,7 +388,7 @@ | |||
258 | 370 | }, nil | 388 | }, nil |
259 | 371 | } | 389 | } |
260 | 372 | 390 | ||
262 | 373 | func setPassword(e state.AuthEntity, password string) error { | 391 | func setPassword(e state.Entity, password string) error { |
263 | 374 | // Catch expected common case of mispelled | 392 | // Catch expected common case of mispelled |
264 | 375 | // or missing Password parameter. | 393 | // or missing Password parameter. |
265 | 376 | if password == "" { | 394 | if password == "" { |
266 | @@ -433,14 +451,14 @@ | |||
267 | 433 | // its methods concurrently. | 451 | // its methods concurrently. |
268 | 434 | type authUser struct { | 452 | type authUser struct { |
269 | 435 | mu sync.Mutex | 453 | mu sync.Mutex |
271 | 436 | _entity state.AuthEntity // logged-in entity (access only when mu is locked) | 454 | _entity state.Entity // logged-in entity (access only when mu is locked) |
272 | 437 | } | 455 | } |
273 | 438 | 456 | ||
274 | 439 | // login authenticates as entity with the given name,. | 457 | // login authenticates as entity with the given name,. |
275 | 440 | func (u *authUser) login(st *state.State, entityName, password string) error { | 458 | func (u *authUser) login(st *state.State, entityName, password string) error { |
276 | 441 | u.mu.Lock() | 459 | u.mu.Lock() |
277 | 442 | defer u.mu.Unlock() | 460 | defer u.mu.Unlock() |
279 | 443 | entity, err := st.AuthEntity(entityName) | 461 | entity, err := st.Entity(entityName) |
280 | 444 | if err != nil && !state.IsNotFound(err) { | 462 | if err != nil && !state.IsNotFound(err) { |
281 | 445 | return err | 463 | return err |
282 | 446 | } | 464 | } |
283 | @@ -463,7 +481,7 @@ | |||
284 | 463 | // entity returns the currently logged-in entity, or nil if not | 481 | // entity returns the currently logged-in entity, or nil if not |
285 | 464 | // currently logged on. The returned entity should not be modified | 482 | // currently logged on. The returned entity should not be modified |
286 | 465 | // because it may be used concurrently. | 483 | // because it may be used concurrently. |
288 | 466 | func (u *authUser) entity() state.AuthEntity { | 484 | func (u *authUser) entity() state.Entity { |
289 | 467 | u.mu.Lock() | 485 | u.mu.Lock() |
290 | 468 | defer u.mu.Unlock() | 486 | defer u.mu.Unlock() |
291 | 469 | return u._entity | 487 | return u._entity |
292 | @@ -471,7 +489,7 @@ | |||
293 | 471 | 489 | ||
294 | 472 | // isMachineWithJob returns whether the given entity is a machine that | 490 | // isMachineWithJob returns whether the given entity is a machine that |
295 | 473 | // is configured to run the given job. | 491 | // is configured to run the given job. |
297 | 474 | func isMachineWithJob(e state.AuthEntity, j state.MachineJob) bool { | 492 | func isMachineWithJob(e state.Entity, j state.MachineJob) bool { |
298 | 475 | m, ok := e.(*state.Machine) | 493 | m, ok := e.(*state.Machine) |
299 | 476 | if !ok { | 494 | if !ok { |
300 | 477 | return false | 495 | return false |
301 | @@ -485,7 +503,7 @@ | |||
302 | 485 | } | 503 | } |
303 | 486 | 504 | ||
304 | 487 | // isAgent returns whether the given entity is an agent. | 505 | // isAgent returns whether the given entity is an agent. |
306 | 488 | func isAgent(e state.AuthEntity) bool { | 506 | func isAgent(e state.Entity) bool { |
307 | 489 | _, isUser := e.(*state.User) | 507 | _, isUser := e.(*state.User) |
308 | 490 | return !isUser | 508 | return !isUser |
309 | 491 | } | 509 | } |
310 | 492 | 510 | ||
311 | === modified file 'state/machine_test.go' | |||
312 | --- state/machine_test.go 2013-03-05 15:26:26 +0000 | |||
313 | +++ state/machine_test.go 2013-03-08 21:34:19 +0000 | |||
314 | @@ -111,7 +111,7 @@ | |||
315 | 111 | } | 111 | } |
316 | 112 | 112 | ||
317 | 113 | func (s *MachineSuite) TestSetPassword(c *C) { | 113 | func (s *MachineSuite) TestSetPassword(c *C) { |
319 | 114 | testSetPassword(c, func() (state.AuthEntity, error) { | 114 | testSetPassword(c, func() (state.Entity, error) { |
320 | 115 | return s.State.Machine(s.machine.Id()) | 115 | return s.State.Machine(s.machine.Id()) |
321 | 116 | }) | 116 | }) |
322 | 117 | } | 117 | } |
323 | 118 | 118 | ||
324 | === modified file 'state/service.go' | |||
325 | --- state/service.go 2013-03-08 17:56:51 +0000 | |||
326 | +++ state/service.go 2013-03-08 21:34:19 +0000 | |||
327 | @@ -52,6 +52,25 @@ | |||
328 | 52 | return s.doc.Name | 52 | return s.doc.Name |
329 | 53 | } | 53 | } |
330 | 54 | 54 | ||
331 | 55 | // EntityName returns a name identifying the service that is safe to use | ||
332 | 56 | // as a file name. The returned name will be different from other | ||
333 | 57 | // EntityName values returned by any other entities from the same state. | ||
334 | 58 | func (s *Service) EntityName() string { | ||
335 | 59 | return "service-" + s.Name() | ||
336 | 60 | } | ||
337 | 61 | |||
338 | 62 | // PasswordValid currently just returns false. Implemented here so that | ||
339 | 63 | // a service can be used as an Entity. | ||
340 | 64 | func (s *Service) PasswordValid(password string) bool { | ||
341 | 65 | return false | ||
342 | 66 | } | ||
343 | 67 | |||
344 | 68 | // SetPassword currently just returns an error. Implemented here so that | ||
345 | 69 | // a service can be used as an Entity. | ||
346 | 70 | func (s *Service) SetPassword(password string) error { | ||
347 | 71 | return fmt.Errorf("cannot set password of service") | ||
348 | 72 | } | ||
349 | 73 | |||
350 | 55 | // Annotations returns the service annotations. | 74 | // Annotations returns the service annotations. |
351 | 56 | func (s *Service) Annotations() map[string]string { | 75 | func (s *Service) Annotations() map[string]string { |
352 | 57 | return s.doc.Annotations | 76 | return s.doc.Annotations |
353 | 58 | 77 | ||
354 | === modified file 'state/service_test.go' | |||
355 | --- state/service_test.go 2013-03-08 19:55:44 +0000 | |||
356 | +++ state/service_test.go 2013-03-08 21:34:19 +0000 | |||
357 | @@ -64,6 +64,10 @@ | |||
358 | 64 | } | 64 | } |
359 | 65 | } | 65 | } |
360 | 66 | 66 | ||
361 | 67 | func (s *ServiceSuite) TestEntityName(c *C) { | ||
362 | 68 | c.Assert(s.mysql.EntityName(), Equals, "service-mysql") | ||
363 | 69 | } | ||
364 | 70 | |||
365 | 67 | func (s *ServiceSuite) TestMysqlEndpoints(c *C) { | 71 | func (s *ServiceSuite) TestMysqlEndpoints(c *C) { |
366 | 68 | _, err := s.mysql.Endpoint("mysql") | 72 | _, err := s.mysql.Endpoint("mysql") |
367 | 69 | c.Assert(err, ErrorMatches, `service "mysql" has no "mysql" relation`) | 73 | c.Assert(err, ErrorMatches, `service "mysql" has no "mysql" relation`) |
368 | 70 | 74 | ||
369 | === modified file 'state/state.go' | |||
370 | --- state/state.go 2013-03-08 19:27:06 +0000 | |||
371 | +++ state/state.go 2013-03-08 21:34:19 +0000 | |||
372 | @@ -274,17 +274,19 @@ | |||
373 | 274 | return newMachine(st, mdoc), nil | 274 | return newMachine(st, mdoc), nil |
374 | 275 | } | 275 | } |
375 | 276 | 276 | ||
379 | 277 | // AuthEntity represents an entity that has | 277 | // Entity represents an entity capabable of handling password authentication |
380 | 278 | // a password that can be authenticated against. | 278 | // and annotations. |
381 | 279 | type AuthEntity interface { | 279 | type Entity interface { |
382 | 280 | EntityName() string | 280 | EntityName() string |
383 | 281 | SetPassword(pass string) error | 281 | SetPassword(pass string) error |
384 | 282 | PasswordValid(pass string) bool | 282 | PasswordValid(pass string) bool |
385 | 283 | Refresh() error | 283 | Refresh() error |
386 | 284 | SetAnnotation(key, value string) error | ||
387 | 285 | Annotations() map[string]string | ||
388 | 284 | } | 286 | } |
389 | 285 | 287 | ||
392 | 286 | // AuthEntity returns the entity for the given name. | 288 | // Entity returns the entity for the given name. |
393 | 287 | func (st *State) AuthEntity(entityName string) (AuthEntity, error) { | 289 | func (st *State) Entity(entityName string) (Entity, error) { |
394 | 288 | i := strings.Index(entityName, "-") | 290 | i := strings.Index(entityName, "-") |
395 | 289 | if i <= 0 || i >= len(entityName)-1 { | 291 | if i <= 0 || i >= len(entityName)-1 { |
396 | 290 | return nil, fmt.Errorf("invalid entity name %q", entityName) | 292 | return nil, fmt.Errorf("invalid entity name %q", entityName) |
397 | @@ -308,6 +310,11 @@ | |||
398 | 308 | return st.Unit(name) | 310 | return st.Unit(name) |
399 | 309 | case "user": | 311 | case "user": |
400 | 310 | return st.User(id) | 312 | return st.User(id) |
401 | 313 | case "service": | ||
402 | 314 | if !IsServiceName(id) { | ||
403 | 315 | return nil, fmt.Errorf("invalid entity name %q", entityName) | ||
404 | 316 | } | ||
405 | 317 | return st.Service(id) | ||
406 | 311 | } | 318 | } |
407 | 312 | return nil, fmt.Errorf("invalid entity name %q", entityName) | 319 | return nil, fmt.Errorf("invalid entity name %q", entityName) |
408 | 313 | } | 320 | } |
409 | 314 | 321 | ||
410 | === modified file 'state/state_test.go' | |||
411 | --- state/state_test.go 2013-03-07 16:05:22 +0000 | |||
412 | +++ state/state_test.go 2013-03-08 21:34:19 +0000 | |||
413 | @@ -1194,7 +1194,7 @@ | |||
414 | 1194 | c.Assert(err, ErrorMatches, "no reachable servers") | 1194 | c.Assert(err, ErrorMatches, "no reachable servers") |
415 | 1195 | } | 1195 | } |
416 | 1196 | 1196 | ||
418 | 1197 | func testSetPassword(c *C, getEntity func() (state.AuthEntity, error)) { | 1197 | func testSetPassword(c *C, getEntity func() (state.Entity, error)) { |
419 | 1198 | e, err := getEntity() | 1198 | e, err := getEntity() |
420 | 1199 | c.Assert(err, IsNil) | 1199 | c.Assert(err, IsNil) |
421 | 1200 | 1200 | ||
422 | @@ -1317,30 +1317,30 @@ | |||
423 | 1317 | c.Assert(err, IsNil) | 1317 | c.Assert(err, IsNil) |
424 | 1318 | } | 1318 | } |
425 | 1319 | 1319 | ||
428 | 1320 | func (s *StateSuite) TestAuthEntity(c *C) { | 1320 | func (s *StateSuite) TestEntity(c *C) { |
429 | 1321 | bad := []string{"", "machine", "-foo", "foo-", "---", "machine-jim", "unit-123", "unit-foo"} | 1321 | bad := []string{"", "machine", "-foo", "foo-", "---", "machine-jim", "unit-123", "unit-foo", "service-", "service-foo/bar"} |
430 | 1322 | for _, name := range bad { | 1322 | for _, name := range bad { |
432 | 1323 | e, err := s.State.AuthEntity(name) | 1323 | e, err := s.State.Entity(name) |
433 | 1324 | c.Check(e, IsNil) | 1324 | c.Check(e, IsNil) |
434 | 1325 | c.Assert(err, ErrorMatches, `invalid entity name ".*"`) | 1325 | c.Assert(err, ErrorMatches, `invalid entity name ".*"`) |
435 | 1326 | } | 1326 | } |
436 | 1327 | 1327 | ||
438 | 1328 | e, err := s.State.AuthEntity("machine-1234") | 1328 | e, err := s.State.Entity("machine-1234") |
439 | 1329 | c.Check(e, IsNil) | 1329 | c.Check(e, IsNil) |
440 | 1330 | c.Assert(err, ErrorMatches, `machine 1234 not found`) | 1330 | c.Assert(err, ErrorMatches, `machine 1234 not found`) |
441 | 1331 | c.Assert(state.IsNotFound(err), Equals, true) | 1331 | c.Assert(state.IsNotFound(err), Equals, true) |
442 | 1332 | 1332 | ||
444 | 1333 | e, err = s.State.AuthEntity("unit-foo-654") | 1333 | e, err = s.State.Entity("unit-foo-654") |
445 | 1334 | c.Check(e, IsNil) | 1334 | c.Check(e, IsNil) |
446 | 1335 | c.Assert(err, ErrorMatches, `unit "foo/654" not found`) | 1335 | c.Assert(err, ErrorMatches, `unit "foo/654" not found`) |
447 | 1336 | c.Assert(state.IsNotFound(err), Equals, true) | 1336 | c.Assert(state.IsNotFound(err), Equals, true) |
448 | 1337 | 1337 | ||
450 | 1338 | e, err = s.State.AuthEntity("unit-foo-bar-654") | 1338 | e, err = s.State.Entity("unit-foo-bar-654") |
451 | 1339 | c.Check(e, IsNil) | 1339 | c.Check(e, IsNil) |
452 | 1340 | c.Assert(err, ErrorMatches, `unit "foo-bar/654" not found`) | 1340 | c.Assert(err, ErrorMatches, `unit "foo-bar/654" not found`) |
453 | 1341 | c.Assert(state.IsNotFound(err), Equals, true) | 1341 | c.Assert(state.IsNotFound(err), Equals, true) |
454 | 1342 | 1342 | ||
456 | 1343 | e, err = s.State.AuthEntity("user-arble") | 1343 | e, err = s.State.Entity("user-arble") |
457 | 1344 | c.Check(e, IsNil) | 1344 | c.Check(e, IsNil) |
458 | 1345 | c.Assert(err, ErrorMatches, `user "arble" not found`) | 1345 | c.Assert(err, ErrorMatches, `user "arble" not found`) |
459 | 1346 | c.Assert(state.IsNotFound(err), Equals, true) | 1346 | c.Assert(state.IsNotFound(err), Equals, true) |
460 | @@ -1348,17 +1348,23 @@ | |||
461 | 1348 | m, err := s.State.AddMachine("series", state.JobHostUnits) | 1348 | m, err := s.State.AddMachine("series", state.JobHostUnits) |
462 | 1349 | c.Assert(err, IsNil) | 1349 | c.Assert(err, IsNil) |
463 | 1350 | 1350 | ||
465 | 1351 | e, err = s.State.AuthEntity(m.EntityName()) | 1351 | e, err = s.State.Entity(m.EntityName()) |
466 | 1352 | c.Assert(err, IsNil) | 1352 | c.Assert(err, IsNil) |
467 | 1353 | c.Assert(e, FitsTypeOf, m) | 1353 | c.Assert(e, FitsTypeOf, m) |
468 | 1354 | c.Assert(e.EntityName(), Equals, m.EntityName()) | 1354 | c.Assert(e.EntityName(), Equals, m.EntityName()) |
469 | 1355 | 1355 | ||
470 | 1356 | svc, err := s.State.AddService("ser-vice1", s.AddTestingCharm(c, "dummy")) | 1356 | svc, err := s.State.AddService("ser-vice1", s.AddTestingCharm(c, "dummy")) |
471 | 1357 | c.Assert(err, IsNil) | 1357 | c.Assert(err, IsNil) |
472 | 1358 | |||
473 | 1359 | service, err := s.State.Entity(svc.EntityName()) | ||
474 | 1360 | c.Assert(err, IsNil) | ||
475 | 1361 | c.Assert(service, FitsTypeOf, svc) | ||
476 | 1362 | c.Assert(service.EntityName(), Equals, svc.EntityName()) | ||
477 | 1363 | |||
478 | 1358 | u, err := svc.AddUnit() | 1364 | u, err := svc.AddUnit() |
479 | 1359 | c.Assert(err, IsNil) | 1365 | c.Assert(err, IsNil) |
480 | 1360 | 1366 | ||
482 | 1361 | e, err = s.State.AuthEntity(u.EntityName()) | 1367 | e, err = s.State.Entity(u.EntityName()) |
483 | 1362 | c.Assert(err, IsNil) | 1368 | c.Assert(err, IsNil) |
484 | 1363 | c.Assert(e, FitsTypeOf, u) | 1369 | c.Assert(e, FitsTypeOf, u) |
485 | 1364 | c.Assert(e.EntityName(), Equals, u.EntityName()) | 1370 | c.Assert(e.EntityName(), Equals, u.EntityName()) |
486 | @@ -1366,7 +1372,7 @@ | |||
487 | 1366 | user, err := s.State.AddUser("arble", "pass") | 1372 | user, err := s.State.AddUser("arble", "pass") |
488 | 1367 | c.Assert(err, IsNil) | 1373 | c.Assert(err, IsNil) |
489 | 1368 | 1374 | ||
491 | 1369 | e, err = s.State.AuthEntity(user.EntityName()) | 1375 | e, err = s.State.Entity(user.EntityName()) |
492 | 1370 | c.Assert(err, IsNil) | 1376 | c.Assert(err, IsNil) |
493 | 1371 | c.Assert(e, FitsTypeOf, user) | 1377 | c.Assert(e, FitsTypeOf, user) |
494 | 1372 | c.Assert(e.EntityName(), Equals, user.EntityName()) | 1378 | c.Assert(e.EntityName(), Equals, user.EntityName()) |
495 | 1373 | 1379 | ||
496 | === modified file 'state/unit_test.go' | |||
497 | --- state/unit_test.go 2013-03-05 15:26:26 +0000 | |||
498 | +++ state/unit_test.go 2013-03-08 21:34:19 +0000 | |||
499 | @@ -174,7 +174,7 @@ | |||
500 | 174 | } | 174 | } |
501 | 175 | 175 | ||
502 | 176 | func (s *UnitSuite) TestSetPassword(c *C) { | 176 | func (s *UnitSuite) TestSetPassword(c *C) { |
504 | 177 | testSetPassword(c, func() (state.AuthEntity, error) { | 177 | testSetPassword(c, func() (state.Entity, error) { |
505 | 178 | return s.State.Unit(s.unit.Name()) | 178 | return s.State.Unit(s.unit.Name()) |
506 | 179 | }) | 179 | }) |
507 | 180 | } | 180 | } |
508 | 181 | 181 | ||
509 | === modified file 'state/user.go' | |||
510 | --- state/user.go 2013-02-18 17:03:00 +0000 | |||
511 | +++ state/user.go 2013-03-08 21:34:19 +0000 | |||
512 | @@ -79,6 +79,18 @@ | |||
513 | 79 | return "user-" + u.doc.Name | 79 | return "user-" + u.doc.Name |
514 | 80 | } | 80 | } |
515 | 81 | 81 | ||
516 | 82 | // Annotations currently just returns an empty map. Implemented here so that | ||
517 | 83 | // a user can be used as an Entity. | ||
518 | 84 | func (u *User) Annotations() map[string]string { | ||
519 | 85 | return map[string]string{} | ||
520 | 86 | } | ||
521 | 87 | |||
522 | 88 | // SetAnnotation currently just returns an error. Implemented here so that | ||
523 | 89 | // a user can be used as an Entity. | ||
524 | 90 | func (u *User) SetAnnotation(key, value string) error { | ||
525 | 91 | return fmt.Errorf("cannot set annotation of user") | ||
526 | 92 | } | ||
527 | 93 | |||
528 | 82 | // SetPassword sets the password associated with the user. | 94 | // SetPassword sets the password associated with the user. |
529 | 83 | func (u *User) SetPassword(password string) error { | 95 | func (u *User) SetPassword(password string) error { |
530 | 84 | hp := trivial.PasswordHash(password) | 96 | hp := trivial.PasswordHash(password) |
531 | 85 | 97 | ||
532 | === modified file 'state/user_test.go' | |||
533 | --- state/user_test.go 2013-02-06 19:57:33 +0000 | |||
534 | +++ state/user_test.go 2013-03-08 21:34:19 +0000 | |||
535 | @@ -43,7 +43,7 @@ | |||
536 | 43 | u, err := s.State.AddUser("someuser", "") | 43 | u, err := s.State.AddUser("someuser", "") |
537 | 44 | c.Assert(err, IsNil) | 44 | c.Assert(err, IsNil) |
538 | 45 | 45 | ||
540 | 46 | testSetPassword(c, func() (state.AuthEntity, error) { | 46 | testSetPassword(c, func() (state.Entity, error) { |
541 | 47 | return s.State.User(u.Name()) | 47 | return s.State.User(u.Name()) |
542 | 48 | }) | 48 | }) |
543 | 49 | } | 49 | } |
Reviewers: mp+152037_ code.launchpad. net,
Message:
Please take a look.
Description:
Annotations API functionality
Annotations are now exposed through the API via 'GetAnnotations' and
'SetAnnotation'. These allow a user to set an annotation on an Entity
(service,
'unit', 'machine', with environment coming in a future branch), and
retrieve
all annotations on an entity. Tests were added around the added
functionality.
Note that now the service is an Entity, as suggested by Roger.
https:/ /code.launchpad .net/~frankban/ juju-core/ bug-1147138- annotations- api/+merge/ 152037
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/7526043/
Affected files: apiclient. go /api_test. go /apiserver. go test.go test.go
A [revision details]
M state/api/
M state/apiserver
M state/apiserver
M state/machine_
M state/service.go
M state/service_
M state/state.go
M state/state_test.go
M state/unit_test.go
M state/user.go
M state/user_test.go