Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add endpoint binding parsing to charm bundles #186
Conversation
dimitern
reviewed
Jan 6, 2016
| + for name, svc := range verifier.bd.Services { | ||
| + charm, ok := verifier.charms[name] | ||
| + // Only thest the ok path here because the !ok path is tested in verifyServices | ||
| + if ok { |
dimitern
reviewed
Jan 6, 2016
| @@ -141,6 +141,9 @@ type ServiceSpec struct { | ||
| // Storage holds the constraints for storage to assign | ||
| // to units of the service. | ||
| Storage map[string]string `bson:",omitempty" json:",omitempty" yaml:",omitempty"` | ||
| + | ||
| + // EndpointBindings maps how endpoints are bound to spaces | ||
| + EndpointBindings map[string]string `bson:"bindings,omitempty" json:"bindings,omitempty" yaml:"bindings,omitempty"` |
dimitern
Jan 6, 2016
As discussed on IRC I'm surprised Storage ... yaml ",omitempty" works correctly for "storage:", but if it does, let's drop the "bindings" in all 3 cases for consistency.
dooferlad
Jan 6, 2016
Contributor
We want the YAML field to be call "bindings" but, internally, we always have used EndpointBindings, hence the explicit name.
dimitern
Jan 6, 2016
D'oh. Sure, that's what I asked for anyway :) Please, ignore my previous comment about it.
dimitern
reviewed
Jan 6, 2016
| + charm, ok := verifier.charms[name] | ||
| + // Only thest the ok path here because the !ok path is tested in verifyServices | ||
| + if ok { | ||
| + for BoundInterface := range svc.EndpointBindings { |
dimitern
Jan 6, 2016
I think this needs to be called "endpoint", as it's the relation name we're binding to a space, not the interface of the relation. E.g. with a metadata like this:
provides:
db:
interface: mysqland bundle.yaml having:
...
bindings:
db: storage
..."db" (not "mysql" - the interface) is bound to space "storage".
dimitern
reviewed
Jan 6, 2016
| + } | ||
| + } | ||
| + | ||
| + if !ok { |
dimitern
reviewed
Jan 6, 2016
| + if !ok { | ||
| + verifier.addErrorf( | ||
| + "service %s wants to bind to interface %s, "+ | ||
| + "which isn't provided or required by charm", |
dimitern
Jan 6, 2016
I think it will be helpful to include the space name in the error message.
s/to interface %s/endpoint %q to space %q/
s/isn't provided or required by charm/isn't among the defined charm relations/ ?
dimitern
commented
Jan 6, 2016
|
It's nearly there, just a few issues need fixing. |
dimitern
reviewed
Jan 6, 2016
| + if !(matchedProvides || matchedRequires || matchedPeers) { | ||
| + verifier.addErrorf( | ||
| + "service %s wants to bind to endpoint %s to space %s, "+ | ||
| + "which isn't provided or required by charm", |
dimitern
Jan 6, 2016
Strictly speaking, "provided or required" is misleading, as it also checks "peers". So how about "isn't defined by the charm" ?
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-charm |
added a commit
that referenced
this pull request
Jan 6, 2016
dooferlad commentedJan 6, 2016
This change adds the ability to bind an endpoint to a space inside a charm bundle, for example:
This will request that wordpress has its database endpoint (db) attached to the db space and the url endpoint attached to the public space.