Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Added support for ExtraBindings in Meta #191
Conversation
added some commits
Mar 1, 2016
frobware
commented on extra_bindings.go in 8bdf27f
Mar 1, 2016
|
From a UX POV, it would be better to accumulate all errors cases. |
|
I didn't want to complicate the code further, but fair enough - will do as you suggest, as good UX is precious ;) |
jameinel
reviewed
Mar 1, 2016
| + bindingsMap := data.(map[interface{}]interface{}) | ||
| + result := make(map[string]ExtraBinding) | ||
| + for name, _ := range bindingsMap { | ||
| + stringName := name.(string) |
jameinel
Mar 1, 2016
Owner
is it ok to do blind casts like this? Doesn't that cause a panic if it isn't a string?
dimitern
Mar 1, 2016
Yes, because parseMetaExtraBindings (like other similar helpers for resources, payloads, storage, etc.) are called once the top-level charm schema has been Coerce()d, which guarantees "extra-bindings" will be a schema.Map() (as defined above, including having non-empty string keys) or else parseMeta() and this func won't be even called. There is also a test for a non-map extra-bindings.
|
I had one comment, but otherwise LGTM |
frobware
commented on extra_bindings.go in ccebeab
Mar 1, 2016
|
"that is not a relation." |
|
Will do. |
frobware
commented on extra_bindings.go in ccebeab
Mar 1, 2016
|
"may" or "should". If "may" what's supported? |
|
Neither - just forgot to update this doc comment. It should be "must not match..." |
frobware
commented on extra_bindings.go in ccebeab
Mar 1, 2016
|
typo "valueLabel" |
|
Will fix this. |
frobware
commented on extra_bindings.go in ccebeab
Mar 1, 2016
|
Seems to read better as "missing binding name" |
|
Sure, sounds better. |
frobware
commented on extra_bindings_test.go in ccebeab
Mar 1, 2016
|
Do these values help trigger/test all the type.(string) assertions? |
|
Actually, this test is no longer needed, as the same thing is more extensively tested in schema.Nil() tests. |
frobware
commented on meta.go in ccebeab
Mar 1, 2016
|
Any reason not to assign to meta.extraBindings because if there's an error we return err. (On reflection maybe not possible with the ':='). |
|
Indeed - the := makes it awkward as it's either this or the equivalent: var err error
meta.ExtraBindings, err = parseMetaExtraBindings(m["extra-bindings"])
if err != nil {
return nil, err
}Perhaps the above is better though, will change. |
frobware
commented on ccebeab
Mar 1, 2016
|
Some cosmetic quibbles/typos, but LGTM. |
dimitern
commented
Mar 1, 2016
|
Thanks for the reviews! All issues fixed, setting to $$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
Mar 1, 2016
jujubot
merged commit 8edb23e
into
juju:v6-unstable
Mar 1, 2016
dimitern
deleted the
dimitern:v6-unstable-extra-bindings
branch
Mar 1, 2016
dimitern
referenced this pull request
Mar 1, 2016
Merged
Added support for extra-bindings within BundleData #192
added a commit
that referenced
this pull request
Mar 2, 2016
dimitern commentedMar 1, 2016
Added an optional
extra-bindingsmetadata section, with the following format:None of the names in the extra-bindings map can match existing provided/required/peer relation
names, as relations are already bindable. Extra bindings provide a way to declare named endpoints,
which the charm needs (bound) addresses for, but which are not relations otherwise.
Extra bindings will be accepted in $ juju deploy --bind ... when declared, in a follow-up.
Also added a Meta.CombinedRelations() helper, fixing http://pad.lv/1520623 bug as a result.