Merge lp:~themue/pyjuju/go into lp:pyjuju/go

[Frank Mueller]

Added ResolvedWatcher for Units.

It reads the resolved mode out of the node content if
it's created or changed.

https://codereview.appspot.com/6120045/

[Gustavo Niemeyer (niemeyer)]

Very nice. Just doc suggestions, except for the missing ok check,
similar to what was done in the other branches.

https://codereview.appspot.com/6120045/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6120045/diff/1/state/unit.go#newcode470
state/unit.go:470: // and checks if it's valid.
// parseResolvedMode returns the resolved mode serialized
// in yaml if it is valid, or an error otherwise.

https://codereview.appspot.com/6120045/diff/1/state/unit.go#newcode486
state/unit.go:486: // resolved mode are used.
// validResolvedMode returns an error if the provided
// mode isn't valid. ResolvedNone is only considered a
// valid mode if acceptNone is true.

https://codereview.appspot.com/6120045/diff/1/state/unit.go#newcode563
state/unit.go:563: // ResolvedWatcher observes changes to a resolved
flag node.
// ResolvedWatcher observes changes to a unit's resolved
// mode. See SetResolved for details.

https://codereview.appspot.com/6120045/diff/1/state/unit.go#newcode573
state/unit.go:573: // watcher for the given path.
// newResolvedWatcher returns a new ResolvedWatcher watching path.

https://codereview.appspot.com/6120045/diff/1/state/unit.go#newcode613
state/unit.go:613: case change := <-w.watcher.Changes():
This needs to check for ok in a similar way to what was done in the
other watchers.

https://codereview.appspot.com/6120045/

[Gustavo Niemeyer (niemeyer)]

LGTM, thanks!

https://codereview.appspot.com/6120045/

[Gustavo Niemeyer (niemeyer)]

<niemeyer> TheMue: You've just reproposed a branch that has already been
submitted: https://codereview.appspot.com/6120045/
<niemeyer> TheMue: Probably because of the vague branch name ("go")

https://codereview.appspot.com/6120045/

Unmerged revisions

202. By Frank Mueller on 2012-06-06

state: Merged prerequisite.

201. By Frank Mueller on 2012-06-06

state: Preparing for prerequisite.

=== modified file 'state/relation.go'
--- state/relation.go	2012-05-31 16:19:15 +0000
+++ state/relation.go	2012-06-06 13:01:07 +0000
@@ -1,6 +1,8 @@
 package state
 
-import ()
+import (
+	"fmt"
+)
 
 // RelationRole defines the role of a relation endpoint.
 type RelationRole string
@@ -47,6 +49,24 @@
 	return e.ServiceName + ":" + e.RelationName
 }
 
+// checkValues performs basic tests on the endpoint data without
+// checking the validity of identifiers.
+func (e *RelationEndpoint) checkValues() error {
+	switch {
+	case e.ServiceName == "":
+		return fmt.Errorf("endpoint has empty service name")
+	case e.Interface == "":
+		return fmt.Errorf("endpoint has empty interface")
+	case e.RelationName == "":
+		return fmt.Errorf("endpoint has empty relation name")
+	case e.RelationRole != RoleProvider && e.RelationRole != RoleRequirer && e.RelationRole != RolePeer:
+		return fmt.Errorf("endpoint has invalid role: %q", e.RelationRole)
+	case e.RelationScope != ScopeGlobal && e.RelationScope != ScopeContainer:
+		return fmt.Errorf("endpoint has invalid scope: %q", e.RelationScope)
+	}
+	return nil
+}
+
 // Relation represents a link between services, or within a
 // service (in the case of peer relations).
 type Relation struct {

=== modified file 'state/state.go'
--- state/state.go	2012-06-05 21:31:50 +0000
+++ state/state.go	2012-06-06 13:01:07 +0000
@@ -312,11 +312,19 @@
 		return nil, nil, fmt.Errorf("relation already exists")
 	}
 	scope := ScopeGlobal
+	serviceKeys := make(map[RelationEndpoint]string)
 	for _, endpoint := range endpoints {
 		if endpoint.RelationScope == ScopeContainer {
 			scope = ScopeContainer
-			break
-		}
+		}
+		if err := endpoint.checkValues(); err != nil {
+			return nil, nil, err
+		}
+		serviceKey, err := t.ServiceKey(endpoint.ServiceName)
+		if err != nil {
+			return nil, nil, err
+		}
+		serviceKeys[endpoint] = serviceKey
 	}
 	// Add a new relation node depending on the scope. Afterwards
 	// create a node and a service relation per endpoint.
@@ -326,10 +334,6 @@
 	}
 	serviceRelations := []*ServiceRelation{}
 	for _, endpoint := range endpoints {
-		serviceKey, err := t.ServiceKey(endpoint.ServiceName)
-		if err != nil {
-			return nil, nil, err
-		}
 		// The relation endpoint node is only created if the scope is 
 		// global. In case of container scoped relations the creation 
 		// per container occurs in ServiceRelation.AddUnit.
@@ -341,7 +345,7 @@
 		serviceRelations = append(serviceRelations, &ServiceRelation{
 			st:            s,
 			relationKey:   relationKey,
-			serviceKey:    serviceKey,
+			serviceKey:    serviceKeys[endpoint],
 			relationScope: endpoint.RelationScope,
 			relationRole:  endpoint.RelationRole,
 			relationName:  endpoint.RelationName,

=== modified file 'state/state_test.go'
--- state/state_test.go	2012-06-06 10:17:40 +0000
+++ state/state_test.go	2012-06-06 13:01:07 +0000
@@ -1085,7 +1085,6 @@
 	c.Assert(serviceRelations[0].RelationScope(), Equals, state.ScopeGlobal)
 	c.Assert(serviceRelations[0].RelationRole(), Equals, state.RolePeer)
 	c.Assert(serviceRelations[0].RelationName(), Equals, "cache")
-
 }
 
 func (s *StateSuite) TestAddRelationMissingService(c *C) {
@@ -1169,3 +1168,28 @@
 	c.Assert(err, IsNil)
 	c.Assert(env.Map(), DeepEquals, map[string]interface{}{"type": "dummy", "name": "foo"})
 }
+
+func (s *StateSuite) TestAddRelationInvalidEndpoints(c *C) {
+	dummy := s.addDummyCharm(c)
+	s.st.AddService("riak", dummy)
+	// Empty service name.
+	riakep := state.RelationEndpoint{"", "ring", "cache", state.RolePeer, state.ScopeGlobal}
+	_, _, err := s.st.AddRelation(riakep)
+	c.Assert(err, ErrorMatches, `endpoint has empty service name`)
+	// Empty interface.
+	riakep = state.RelationEndpoint{"riak", "", "cache", state.RolePeer, state.ScopeGlobal}
+	_, _, err = s.st.AddRelation(riakep)
+	c.Assert(err, ErrorMatches, `endpoint has empty interface`)
+	// Empty relation name.
+	riakep = state.RelationEndpoint{"riak", "ring", "", state.RolePeer, state.ScopeGlobal}
+	_, _, err = s.st.AddRelation(riakep)
+	c.Assert(err, ErrorMatches, `endpoint has empty relation name`)
+	// Invalid role.
+	riakep = state.RelationEndpoint{"riak", "ring", "cache", state.RelationRole("foo"), state.ScopeGlobal}
+	_, _, err = s.st.AddRelation(riakep)
+	c.Assert(err, ErrorMatches, `can't add non-peer relation with a single service`)
+	// Invalid role.
+	riakep = state.RelationEndpoint{"riak", "ring", "cache", state.RolePeer, state.RelationScope("foo")}
+	_, _, err = s.st.AddRelation(riakep)
+	c.Assert(err, ErrorMatches, `endpoint has invalid scope: "foo"`)
+}