juju-core

Merge lp:~dimitern/juju-core/103-relation-entity-tags into lp:~go-bot/juju-core/trunk

  1. 103-relation-entity-tags
  2. Merge into trunk
Proposed by Dimiter Naydenov
Status: Merged
Approved by: Dimiter Naydenov
Approved revision: no longer in the source branch.
Merged at revision: 1661
Proposed branch: lp:~dimitern/juju-core/103-relation-entity-tags
Merge into: lp:~go-bot/juju-core/trunk
Prerequisite: lp:~dimitern/juju-core/102-state-endpoint-refactoring
Diff against target: 277 lines (+124/-23)
7 files modified
names/relation.go (+24/-0)
names/relation_test.go (+36/-0)
names/tag.go (+18/-15)
names/tag_test.go (+9/-3)
state/relation.go (+7/-0)
state/state.go (+9/-0)
state/state_test.go (+21/-5)
To merge this branch: bzr merge lp:~dimitern/juju-core/103-relation-entity-tags
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+180157@code.launchpad.net

Commit message

state: Make Relation an Entity

This adds relation tags to the names package. The
format is "relation-<id>", where <id> is the relation's
numeric id.
It also adds a Tag() method on the relation, and a few
other necessary modifications to make state.Relation
objects behave like other entities. This is needed for
the uniter API facade, so relations can be used as
params and results.

https://codereview.appspot.com/12748044/

R=gz, rogpeppe

Description of the change

state: Make Relation an Entity

This adds relation tags to the names package. The
format is "relation-<id>", where <id> is the relation's
numeric id.
It also adds a Tag() method on the relation, and a few
other necessary modifications to make state.Relation
objects behave like other entities. This is needed for
the uniter API facade, so relations can be used as
params and results.

https://codereview.appspot.com/12748044/

To post a comment you must log in.
Revision history for this message
Dimiter Naydenov (dimitern) wrote :

Reviewers: mp+180157_code.launchpad.net,

Message:
Please take a look.

Description:
state: Make Relation an Entity

This adds relation tags to the names package. The
format is "relation-<id>", where <id> is the relation's
numeric id.
It also adds a Tag() method on the relation, and a few
other necessary modifications to make state.Relation
objects behave like other entities. This is needed for
the uniter API facade, so relations can be used as
params and results.

https://code.launchpad.net/~dimitern/juju-core/103-relation-entity-tags/+merge/180157

Requires:
https://code.launchpad.net/~dimitern/juju-core/102-state-endpoint-refactoring/+merge/180141

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   A names/relation.go
   A names/relation_test.go
   M names/tag.go
   M names/tag_test.go
   M state/relation.go
   M state/state.go
   M state/state_test.go

Revision history for this message
Roger Peppe (rogpeppe) wrote :

LGTM with some extra tests.

https://codereview.appspot.com/12748044/diff/1/names/relation.go
File names/relation.go (right):

https://codereview.appspot.com/12748044/diff/1/names/relation.go#newcode18
names/relation.go:18: func RelationTag(relationId string) string {
if !IsRelation(relationId) {
    panic(fmt.Sprintf("%q is not a valid relation id", relationId))
}

https://codereview.appspot.com/12748044/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/12748044/diff/1/state/relation.go#newcode66
state/relation.go:66: func (r *Relation) Tag() string {
This should have a test.

https://codereview.appspot.com/12748044/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/12748044/diff/1/state/state_test.go#newcode1438
state/state_test.go:1438: tag: "service-ser-vice2",
We need a relation tag test here.

https://codereview.appspot.com/12748044/

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

Please take a look.

https://codereview.appspot.com/12748044/diff/1/names/relation.go
File names/relation.go (right):

https://codereview.appspot.com/12748044/diff/1/names/relation.go#newcode18
names/relation.go:18: func RelationTag(relationId string) string {
On 2013/08/14 15:02:49, rog wrote:
> if !IsRelation(relationId) {
> panic(fmt.Sprintf("%q is not a valid relation id", relationId))
> }

Done.

https://codereview.appspot.com/12748044/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/12748044/diff/1/state/relation.go#newcode66
state/relation.go:66: func (r *Relation) Tag() string {
On 2013/08/14 15:02:49, rog wrote:
> This should have a test.

Done.

https://codereview.appspot.com/12748044/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/12748044/diff/1/state/state_test.go#newcode1438
state/state_test.go:1438: tag: "service-ser-vice2",
On 2013/08/14 15:02:49, rog wrote:
> We need a relation tag test here.

Done.

https://codereview.appspot.com/12748044/

Revision history for this message
Martin Packman (gz) wrote :

LGTM.

https://codereview.appspot.com/12748044/diff/5002/names/relation_test.go
File names/relation_test.go (right):

https://codereview.appspot.com/12748044/diff/5002/names/relation_test.go#newcode16
names/relation_test.go:16: var relationIdTests = []struct {
Okay, so this is a case where I like the table test style.

https://codereview.appspot.com/12748044/diff/5002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/12748044/diff/5002/state/state.go#newcode498
state/state.go:498: return nil, errors.NotFoundf("relation %s", id)
If the conversion fails, is that really a not found rather than an
invalid situation?

Is there a test exercising this branch?

https://codereview.appspot.com/12748044/

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

Please take a look.

https://codereview.appspot.com/12748044/diff/5002/names/relation_test.go
File names/relation_test.go (right):

https://codereview.appspot.com/12748044/diff/5002/names/relation_test.go#newcode16
names/relation_test.go:16: var relationIdTests = []struct {
On 2013/08/14 15:54:56, gz wrote:
> Okay, so this is a case where I like the table test style.

Yeah!

https://codereview.appspot.com/12748044/diff/5002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/12748044/diff/5002/state/state.go#newcode498
state/state.go:498: return nil, errors.NotFoundf("relation %s", id)
On 2013/08/14 15:54:56, gz wrote:
> If the conversion fails, is that really a not found rather than an
 invalid
> situation?

> Is there a test exercising this branch?

I'll add a test, thanks. And the point is: there won't be a relation
with an invalid id anyway, so it's NotFoundError. It will also help
later in the API when we convert NotFoundErrors to ErrPerm.

https://codereview.appspot.com/12748044/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== added file 'names/relation.go'
--- names/relation.go 1970-01-01 00:00:00 +0000
+++ names/relation.go 2013-08-14 16:01:40 +0000
@@ -0,0 +1,24 @@
1// Copyright 2013 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4package names
5
6import (
7 "fmt"
8 "regexp"
9)
10
11var validRelation = regexp.MustCompile("^" + NumberSnippet + "$")
12
13// IsRelation returns whether id is a valid relation id.
14func IsRelation(id string) bool {
15 return validRelation.MatchString(id)
16}
17
18// RelationTag returns the tag for the relation with the given id.
19func RelationTag(relationId string) string {
20 if !IsRelation(relationId) {
21 panic(fmt.Sprintf("%q is not a valid relation id", relationId))
22 }
23 return makeTag(RelationTagKind, relationId)
24}
025
=== added file 'names/relation_test.go'
--- names/relation_test.go 1970-01-01 00:00:00 +0000
+++ names/relation_test.go 2013-08-14 16:01:40 +0000
@@ -0,0 +1,36 @@
1// Copyright 2013 Canonical Ltd.
2// Licensed under the AGPLv3, see LICENCE file for details.
3
4package names_test
5
6import (
7 gc "launchpad.net/gocheck"
8
9 "launchpad.net/juju-core/names"
10)
11
12type relationSuite struct{}
13
14var _ = gc.Suite(&relationSuite{})
15
16var relationIdTests = []struct {
17 pattern string
18 valid bool
19}{
20 {pattern: "", valid: false},
21 {pattern: "foo", valid: false},
22 {pattern: "foo42", valid: false},
23 {pattern: "42", valid: true},
24 {pattern: "0", valid: true},
25 {pattern: "%not", valid: false},
26 {pattern: "42also-not", valid: false},
27 {pattern: "042", valid: false},
28 {pattern: "0x42", valid: false},
29}
30
31func (s *relationSuite) TestRelationIdFormats(c *gc.C) {
32 for i, test := range relationIdTests {
33 c.Logf("test %d: %q", i, test.pattern)
34 c.Assert(names.IsRelation(test.pattern), gc.Equals, test.valid)
35 }
36}
037
=== modified file 'names/tag.go'
--- names/tag.go 2013-08-07 14:21:04 +0000
+++ names/tag.go 2013-08-14 16:01:40 +0000
@@ -9,19 +9,21 @@
9)9)
1010
11const (11const (
12 UnitTagKind = "unit"12 UnitTagKind = "unit"
13 MachineTagKind = "machine"13 MachineTagKind = "machine"
14 ServiceTagKind = "service"14 ServiceTagKind = "service"
15 EnvironTagKind = "environment"15 EnvironTagKind = "environment"
16 UserTagKind = "user"16 UserTagKind = "user"
17 RelationTagKind = "relation"
17)18)
1819
19var validKinds = map[string]bool{20var validKinds = map[string]bool{
20 UnitTagKind: true,21 UnitTagKind: true,
21 MachineTagKind: true,22 MachineTagKind: true,
22 ServiceTagKind: true,23 ServiceTagKind: true,
23 EnvironTagKind: true,24 EnvironTagKind: true,
24 UserTagKind: true,25 UserTagKind: true,
26 RelationTagKind: true,
25}27}
2628
27var tagSuffixToId = map[string]func(string) string{29var tagSuffixToId = map[string]func(string) string{
@@ -30,11 +32,12 @@
30}32}
3133
32var verifyId = map[string]func(string) bool{34var verifyId = map[string]func(string) bool{
33 UnitTagKind: IsUnit,35 UnitTagKind: IsUnit,
34 MachineTagKind: IsMachine,36 MachineTagKind: IsMachine,
35 ServiceTagKind: IsService,37 ServiceTagKind: IsService,
36 UserTagKind: IsUser,38 UserTagKind: IsUser,
37 EnvironTagKind: IsEnvironment,39 EnvironTagKind: IsEnvironment,
40 RelationTagKind: IsRelation,
38}41}
3942
40// TagKind returns one of the *TagKind constants for the given tag, or43// TagKind returns one of the *TagKind constants for the given tag, or
4144
=== modified file 'names/tag_test.go'
--- names/tag_test.go 2013-08-07 14:21:04 +0000
+++ names/tag_test.go 2013-08-14 16:01:40 +0000
@@ -23,6 +23,7 @@
23 {tag: "service-foo", kind: names.ServiceTagKind},23 {tag: "service-foo", kind: names.ServiceTagKind},
24 {tag: "environment-42", kind: names.EnvironTagKind},24 {tag: "environment-42", kind: names.EnvironTagKind},
25 {tag: "user-admin", kind: names.UserTagKind},25 {tag: "user-admin", kind: names.UserTagKind},
26 {tag: "relation-42", kind: names.RelationTagKind},
26 {tag: "foo", err: `"foo" is not a valid tag`},27 {tag: "foo", err: `"foo" is not a valid tag`},
27 {tag: "unit", err: `"unit" is not a valid tag`},28 {tag: "unit", err: `"unit" is not a valid tag`},
28}29}
@@ -95,6 +96,10 @@
95 expectKind: names.EnvironTagKind,96 expectKind: names.EnvironTagKind,
96 resultId: "foo",97 resultId: "foo",
97}, {98}, {
99 tag: "relation-42",
100 expectKind: names.RelationTagKind,
101 resultId: "42",
102}, {
98 tag: "environment-/",103 tag: "environment-/",
99 expectKind: names.EnvironTagKind,104 expectKind: names.EnvironTagKind,
100 resultErr: `"environment-/" is not a valid environment tag`,105 resultErr: `"environment-/" is not a valid environment tag`,
@@ -113,9 +118,10 @@
113}}118}}
114119
115var makeTag = map[string]func(id string) string{120var makeTag = map[string]func(id string) string{
116 names.MachineTagKind: names.MachineTag,121 names.MachineTagKind: names.MachineTag,
117 names.UnitTagKind: names.UnitTag,122 names.UnitTagKind: names.UnitTag,
118 names.ServiceTagKind: names.ServiceTag,123 names.ServiceTagKind: names.ServiceTag,
124 names.RelationTagKind: names.RelationTag,
119 // TODO(rog) environment and user, when they have Tag functions.125 // TODO(rog) environment and user, when they have Tag functions.
120}126}
121127
122128
=== modified file 'state/relation.go'
--- state/relation.go 2013-07-09 10:32:23 +0000
+++ state/relation.go 2013-08-14 16:01:40 +0000
@@ -15,6 +15,7 @@
1515
16 "launchpad.net/juju-core/charm"16 "launchpad.net/juju-core/charm"
17 errors "launchpad.net/juju-core/errors"17 errors "launchpad.net/juju-core/errors"
18 "launchpad.net/juju-core/names"
18 "launchpad.net/juju-core/utils"19 "launchpad.net/juju-core/utils"
19)20)
2021
@@ -60,6 +61,12 @@
60 return r.doc.Key61 return r.doc.Key
61}62}
6263
64// Tag returns a name identifying the relation that is safe to use
65// as a file name.
66func (r *Relation) Tag() string {
67 return names.RelationTag(strconv.Itoa(r.doc.Id))
68}
69
63// Refresh refreshes the contents of the relation from the underlying70// Refresh refreshes the contents of the relation from the underlying
64// state. It returns an error that satisfies IsNotFound if the relation has been71// state. It returns an error that satisfies IsNotFound if the relation has been
65// removed.72// removed.
6673
=== modified file 'state/state.go'
--- state/state.go 2013-08-07 14:23:50 +0000
+++ state/state.go 2013-08-14 16:01:40 +0000
@@ -492,6 +492,12 @@
492 return nil, errors.NotFoundf("environment %q", id)492 return nil, errors.NotFoundf("environment %q", id)
493 }493 }
494 return st.Environment()494 return st.Environment()
495 case names.RelationTagKind:
496 relId, err := strconv.Atoi(id)
497 if err != nil {
498 return nil, errors.NotFoundf("relation %s", id)
499 }
500 return st.Relation(relId)
495 }501 }
496 return nil, err502 return nil, err
497}503}
@@ -512,6 +518,8 @@
512 coll = st.units.Name518 coll = st.units.Name
513 case names.UserTagKind:519 case names.UserTagKind:
514 coll = st.users.Name520 coll = st.users.Name
521 case names.RelationTagKind:
522 coll = st.relations.Name
515 default:523 default:
516 return "", "", fmt.Errorf("%q is not a valid collection tag", tag)524 return "", "", fmt.Errorf("%q is not a valid collection tag", tag)
517 }525 }
@@ -1181,6 +1189,7 @@
1181 's': names.ServiceTagKind + "-",1189 's': names.ServiceTagKind + "-",
1182 'u': names.UnitTagKind + "-",1190 'u': names.UnitTagKind + "-",
1183 'e': names.EnvironTagKind + "-",1191 'e': names.EnvironTagKind + "-",
1192 'r': names.RelationTagKind + "-",
1184}1193}
11851194
1186func tagForGlobalKey(key string) (string, bool) {1195func tagForGlobalKey(key string) (string, bool) {
11871196
=== modified file 'state/state_test.go'
--- state/state_test.go 2013-08-08 17:56:28 +0000
+++ state/state_test.go 2013-08-14 16:01:40 +0000
@@ -1412,6 +1412,12 @@
1412 tag: "unit-123",1412 tag: "unit-123",
1413 err: `"unit-123" is not a valid unit tag`,1413 err: `"unit-123" is not a valid unit tag`,
1414}, {1414}, {
1415 tag: "relation-blah",
1416 err: `"relation-blah" is not a valid relation tag`,
1417}, {
1418 tag: "relation-42",
1419 err: "relation 42 not found",
1420}, {
1415 tag: "unit-foo",1421 tag: "unit-foo",
1416 err: `"unit-foo" is not a valid unit tag`,1422 err: `"unit-foo" is not a valid unit tag`,
1417}, {1423}, {
@@ -1437,6 +1443,8 @@
1437}, {1443}, {
1438 tag: "service-ser-vice2",1444 tag: "service-ser-vice2",
1439}, {1445}, {
1446 tag: "relation-0",
1447}, {
1440 tag: "unit-ser-vice2-0",1448 tag: "unit-ser-vice2-0",
1441}, {1449}, {
1442 tag: "user-arble",1450 tag: "user-arble",
@@ -1445,11 +1453,12 @@
1445}}1453}}
14461454
1447var entityTypes = map[string]interface{}{1455var entityTypes = map[string]interface{}{
1448 names.UserTagKind: (*state.User)(nil),1456 names.UserTagKind: (*state.User)(nil),
1449 names.EnvironTagKind: (*state.Environment)(nil),1457 names.EnvironTagKind: (*state.Environment)(nil),
1450 names.ServiceTagKind: (*state.Service)(nil),1458 names.ServiceTagKind: (*state.Service)(nil),
1451 names.UnitTagKind: (*state.Unit)(nil),1459 names.UnitTagKind: (*state.Unit)(nil),
1452 names.MachineTagKind: (*state.Machine)(nil),1460 names.MachineTagKind: (*state.Machine)(nil),
1461 names.RelationTagKind: (*state.Relation)(nil),
1453}1462}
14541463
1455func (s *StateSuite) TestFindEntity(c *gc.C) {1464func (s *StateSuite) TestFindEntity(c *gc.C) {
@@ -1461,6 +1470,13 @@
1461 c.Assert(err, gc.IsNil)1470 c.Assert(err, gc.IsNil)
1462 _, err = s.State.AddUser("arble", "pass")1471 _, err = s.State.AddUser("arble", "pass")
1463 c.Assert(err, gc.IsNil)1472 c.Assert(err, gc.IsNil)
1473 _, err = s.State.AddService("wordpress", s.AddTestingCharm(c, "wordpress"))
1474 c.Assert(err, gc.IsNil)
1475 eps, err := s.State.InferEndpoints([]string{"wordpress", "ser-vice2"})
1476 c.Assert(err, gc.IsNil)
1477 rel, err := s.State.AddRelation(eps...)
1478 c.Assert(err, gc.IsNil)
1479 c.Assert(rel.Id(), gc.Equals, 0)
14641480
1465 for i, test := range findEntityTests {1481 for i, test := range findEntityTests {
1466 c.Logf("test %d: %q", i, test.tag)1482 c.Logf("test %d: %q", i, test.tag)

Subscribers

People subscribed via source and target branches

to status/vote changes: