Issue 5836049: Add support for using relation ids to refer unambiguously to relations.
DescriptionAdd support for using relation ids to refer unambiguously to relations.
https://code.launchpad.net/~jimbaker/juju/relation-reference-spec/+merge/97721
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 16
Patch Set 2 : Add support for using relation ids to refer unambiguously to relations. #
Total comments: 10
Patch Set 3 : Add support for using relation ids to refer unambiguously to relations. #
MessagesTotal messages: 8
Please take a look.
Sign in to reply to this message.
https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... File source/drafts/relation-reference.rst (right): https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... source/drafts/relation-reference.rst:6: relation name or the relation id: You need to back these statements. The proposal starts by already saying that the feature is required, and that it's going to be required in one of two ways. I suggest dropping part of this introduction and starting the spec at the point mentioned below. https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... source/drafts/relation-reference.rst:18: Relation ids are also sometimes required. This is because a service Start at "[A] service may provide an interface (...)". Here you have a good introduction to why you're proposing a change to the current mechanism. https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... source/drafts/relation-reference.rst:22: ``mysql`` interface to multiple consuming clients using the relation Examples benefit from unique names so that one can tell what is being referred to: (...). For example, the ``mysql5`` service might provide a relation named `db` with a ``mysql`` interface. https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... source/drafts/relation-reference.rst:24: to use its relation id, not the relation name. This sentence has to be more specific. In many circumstances, it is possible to specify a relation solely by its name, even in the scenario you provide. Note that so far we don't have this setting, and we manage to do reasonably well without it. Please finish this paragraph with an actual example that requires such unique id, to give context to the whole proposal. https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... source/drafts/relation-reference.rst:27: as described in a subsequent section. Also needs context. Why is this necessary? You've also mentioned $JUJU_RELATION_ID above. What's the difference between them? The paragraph also needs to be moved. It's saying how a relation id may be obtained, without ever describing what it is, which is done by the following paragraph. https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... source/drafts/relation-reference.rst:30: be ``<interface name>-<normalized internal relation This should probably be <relation name> rather than <interface name> and, it'd be good to avoid the dash between those entries, because relation names are often named after the charm, and "mysql-42" looks like a charm name-revision. Might also look nicer if indented. I suggest something like: """ To ensure readability of this relation id, its format is specified to be:: <relation name>:<normalized internal relation id> Normalization removes any padding zeros from the internal id. For example, if the relation is associated with a relation named ``http`` and the respective relation id is ``relation-0000000042``, then the relation id is ``http:42``. """ Please put the bug in a references section at the end. It's not relevant to the point being made here. https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... source/drafts/relation-reference.rst:42: followed by a number. This is unnecessary given the above suggestion of using a colon. https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... source/drafts/relation-reference.rst:44: Lastly, relation ids are never visible outside of relation hooks or This paragraph must be inverted. It's stating where the id isn't visible, but the spec never describes where the setting *is* visible.
Sign in to reply to this message.
https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... File source/drafts/relation-reference.rst (right): https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... source/drafts/relation-reference.rst:6: relation name or the relation id: Done. https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... source/drafts/relation-reference.rst:18: Relation ids are also sometimes required. This is because a service On 2012/03/15 20:19:48, niemeyer wrote: > Start at "[A] service may provide an interface (...)". Here you have a good > introduction to why you're proposing a change to the current mechanism. Done. https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... source/drafts/relation-reference.rst:22: ``mysql`` interface to multiple consuming clients using the relation On 2012/03/15 20:19:48, niemeyer wrote: > Examples benefit from unique names so that one can tell what is being referred > to: > > (...). For example, the ``mysql5`` service might provide > a relation named `db` with a ``mysql`` interface. Done. https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... source/drafts/relation-reference.rst:24: to use its relation id, not the relation name. On 2012/03/15 20:19:48, niemeyer wrote: > This sentence has to be more specific. In many circumstances, it is possible to > specify a relation solely by its name, even in the scenario you provide. Note > that so far we don't have this setting, and we manage to do reasonably well > without it. Please finish this paragraph with an actual example that requires > such unique id, to give context to the whole proposal. Done. https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... source/drafts/relation-reference.rst:27: as described in a subsequent section. On 2012/03/15 20:19:48, niemeyer wrote: > Also needs context. Why is this necessary? You've also mentioned > $JUJU_RELATION_ID above. What's the difference between them? > > The paragraph also needs to be moved. It's saying how a relation id may be > obtained, without ever describing what it is, which is done by the following > paragraph. Done. https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... source/drafts/relation-reference.rst:30: be ``<interface name>-<normalized internal relation On 2012/03/15 20:19:48, niemeyer wrote: > This should probably be <relation name> rather than <interface name> and, it'd > be good to avoid the dash between those entries, because relation names are > often named after the charm, and "mysql-42" looks like a charm name-revision. > Might also look nicer if indented. > > I suggest something like: > > """ > To ensure readability of this relation id, its format is specified to be:: > > <relation name>:<normalized internal relation id> > > Normalization removes any padding zeros from the internal id. For example, if > the relation is associated with a relation named ``http`` and the respective > relation id is ``relation-0000000042``, then the relation id is ``http:42``. > """ > > Please put the bug in a references section at the end. It's not relevant to the > point being made here. Done. https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... source/drafts/relation-reference.rst:42: followed by a number. On 2012/03/15 20:19:48, niemeyer wrote: > This is unnecessary given the above suggestion of using a colon. Done. https://codereview.appspot.com/5836049/diff/1/source/drafts/relation-referenc... source/drafts/relation-reference.rst:44: Lastly, relation ids are never visible outside of relation hooks or On 2012/03/15 20:19:48, niemeyer wrote: > This paragraph must be inverted. It's stating where the id isn't visible, but > the spec never describes where the setting *is* visible. Done.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Much better, thank you. Just a couple of final details. https://codereview.appspot.com/5836049/diff/5001/source/drafts/relation-refer... File source/drafts/relation-reference.rst (right): https://codereview.appspot.com/5836049/diff/5001/source/drafts/relation-refer... source/drafts/relation-reference.rst:2: ------------------- Please check the other specs to see how they format headers and use the same syntax. I believe it's === for the first header and --- for the internal ones. https://codereview.appspot.com/5836049/diff/5001/source/drafts/relation-refer... source/drafts/relation-reference.rst:35: its relation id, not the relation name. However, the use of relation Remove content after the dot. We'll use it later. https://codereview.appspot.com/5836049/diff/5001/source/drafts/relation-refer... source/drafts/relation-reference.rst:46: addition, a new environment variable, `$JUJU_RELATION_ID`, will always In addition to normalization we're setting an environment variable? That's a different topic. Break line, and bring back the earlier text merged with it: """ Right now relation ids are only visible inside relation hooks, via the $JUJU_RELATION_ID environment variable. In particular, they are not visible in the output of "juju status". """ https://codereview.appspot.com/5836049/diff/5001/source/drafts/relation-refer... source/drafts/relation-reference.rst:62: ~~~~~~~~~~ I believe we use --- for internal headers.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
https://codereview.appspot.com/5836049/diff/5001/source/drafts/relation-refer... File source/drafts/relation-reference.rst (right): https://codereview.appspot.com/5836049/diff/5001/source/drafts/relation-refer... source/drafts/relation-reference.rst:2: ------------------- On 2012/03/20 13:31:22, niemeyer wrote: > Please check the other specs to see how they format headers and use the same > syntax. I believe it's === for the first header and --- for the internal ones. Done. https://codereview.appspot.com/5836049/diff/5001/source/drafts/relation-refer... source/drafts/relation-reference.rst:35: its relation id, not the relation name. However, the use of relation On 2012/03/20 13:31:22, niemeyer wrote: > Remove content after the dot. We'll use it later. Done. https://codereview.appspot.com/5836049/diff/5001/source/drafts/relation-refer... source/drafts/relation-reference.rst:46: addition, a new environment variable, `$JUJU_RELATION_ID`, will always On 2012/03/20 13:31:22, niemeyer wrote: > In addition to normalization we're setting an environment variable? That's a > different topic. Break line, and bring back the earlier text merged with it: > > """ > Right now relation ids are only visible inside relation hooks, via the > $JUJU_RELATION_ID environment variable. In particular, they are not visible in > the output of "juju status". > """ Done. https://codereview.appspot.com/5836049/diff/5001/source/drafts/relation-refer... source/drafts/relation-reference.rst:62: ~~~~~~~~~~ On 2012/03/20 13:31:22, niemeyer wrote: > I believe we use --- for internal headers. Done.
Sign in to reply to this message.
You haven't actually done the changes. I don't care much, though. The meat of the spec look good. LGTM https://codereview.appspot.com/5836049/diff/5001/source/drafts/relation-refer... File source/drafts/relation-reference.rst (right): https://codereview.appspot.com/5836049/diff/5001/source/drafts/relation-refer... source/drafts/relation-reference.rst:35: its relation id, not the relation name. However, the use of relation On 2012/03/22 04:17:53, jimbaker wrote: > On 2012/03/20 13:31:22, niemeyer wrote: > > Remove content after the dot. We'll use it later. > > Done. Not done. https://codereview.appspot.com/5836049/diff/5001/source/drafts/relation-refer... source/drafts/relation-reference.rst:46: addition, a new environment variable, `$JUJU_RELATION_ID`, will always On 2012/03/22 04:17:53, jimbaker wrote: > On 2012/03/20 13:31:22, niemeyer wrote: > > In addition to normalization we're setting an environment variable? That's a > > different topic. Break line, and bring back the earlier text merged with it: > > > > """ > > Right now relation ids are only visible inside relation hooks, via the > > $JUJU_RELATION_ID environment variable. In particular, they are not visible in > > the output of "juju status". > > """ > > Done. Not done.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
