Merge lp://staging/~jimbaker/pyjuju/relation-info-command-spec into lp://staging/~juju/pyjuju/docs

Proposed by Jim Baker
Status: Merged
Approved by: Kapil Thangavelu
Approved revision: 8
Merge reported by: Mark Mims
Merged at revision: not available
Proposed branch: lp://staging/~jimbaker/pyjuju/relation-info-command-spec
Merge into: lp://staging/~juju/pyjuju/docs
Diff against target: 57 lines (+53/-0)
1 file modified
source/drafts/relation-info.rst (+53/-0)
To merge this branch: bzr merge lp://staging/~jimbaker/pyjuju/relation-info-command-spec
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+97736@code.staging.launchpad.net

Description of the change

Add `relation-info` relation hook command.

https://codereview.appspot.com/5837050/

To post a comment you must log in.
Revision history for this message
Jim Baker (jimbaker) wrote :
Download full text (3.4 KiB)

Reviewers: mp+97736_code.launchpad.net,

Message:
Please take a look.

Description:
Add `relation-info` relation hook command.

https://code.launchpad.net/~jimbaker/juju/relation-info-command-spec/+merge/97736

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   A source/drafts/relation-info.rst

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: source/drafts/relation-info.rst
=== added file 'source/drafts/relation-info.rst'
--- source/drafts/relation-info.rst 1970-01-01 00:00:00 +0000
+++ source/drafts/relation-info.rst 2012-03-15 19:42:23 +0000
@@ -0,0 +1,68 @@
+New `relation-info` hook command
+--------------------------------
+
+This proposal adds a new relation hook command to enumerate the
+relations a service participates in::
+
+ relation-info [--interface INTERFACE] [-r RELATION_NAME_OR_ID]
+ [SERVICE_OR_UNIT_NAME]
+
+The output is a whitespace separated list of relation ids, if any.
+
+The service name is defaulted to the one for which this hook is
+running. Using unit name instead is allowed as a convenience: only the
+service portion is meaningful. In the case of using this command
+outside of a relation hook, the service or unit name must be
+specified.
+
+Notes:
+
+ * Since the interface is encoded in the relation id, it is also
+ possible to filter that as desired without using the `--interface`
+ option.
+
+ * `relation-info` taking a relation id with `-r` can be used to
+ verify the continued existence of this relation id for the
+ service. If `-r` is specified with a relation name, all
+ corresponding relation ids for that name are returned.
+
+ * For any hook, the data for the `relation-info` for the the
+ corresponding default service is cached prior to calling the hook
+ script. Calling `relation-info` for other services is read at that
+ time and cached for the duration of the hook.
+
+Example. The keystone charm has the following definition in its
+`metadata.yaml`::
+
+ name: keystone
+ summary: Proposed OpenStack identity service - Daemons
+ description: |
+ Proposed OpenStack identity service - Daemons
+ provides:
+ identity-service:
+ interface: keystone
+ keystone-service:
+ interface: keystone
+ requires:
+ shared-db:
+ interface: mysql-shared
+
+For a keystone service unit, `relation-info` can be used to create a
+list of relation ids that provide the keystone interface, then
+enumerated over to set the ready for each relation. Setting this value
+will then trigger `<relation name>-relation-changed` hooks for service
+units on the opposite side of these relations::
+
+ for relation_id in $(relation-info --interface=keystone); do
+ relation-set -r $relation_id ready=true
+ done
+
+In certain cases, this specific triggering could be initiated in this
+example as fo...

Read more...

7. By Jim Baker

Updated to use new relation id format and added back bug reference

Revision history for this message
Jim Baker (jimbaker) wrote :
Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

https://codereview.appspot.com/5837050/diff/3001/source/drafts/relation-info.rst
File source/drafts/relation-info.rst (right):

https://codereview.appspot.com/5837050/diff/3001/source/drafts/relation-info.rst#newcode7
source/drafts/relation-info.rst:7: relation-info [--interface INTERFACE]
[-r RELATION_NAME_OR_ID]
If this command enumerates relation ids, how can it accept a relation id
as a parameter? Also, I don't see how it can take a service name as a
parameter either, or why it takes an interface, or even a unit name.

If I understand what this command is doing, I'd imagine something much
simpler instead:

     relation-ids [<relation name>]

This will list the given relation name, or the relation name currently
executing if the parameter is omitted.

If you have real problems that need a more complex interface than this,
can you please ping me online? I'd be happy to understand them.

https://codereview.appspot.com/5837050/diff/3001/source/drafts/relation-info.rst#newcode21
source/drafts/relation-info.rst:21: possible to filter that as desired
without using the `--interface`
So why do we need it?

https://codereview.appspot.com/5837050/diff/3001/source/drafts/relation-info.rst#newcode25
source/drafts/relation-info.rst:25: verify the continued existence of
this relation id for the
That's relation-ids | grep $R

https://codereview.appspot.com/5837050/diff/3001/source/drafts/relation-info.rst#newcode31
source/drafts/relation-info.rst:31: script. Calling `relation-info` for
other services is read at that
Why do we need to ask about relation ids for other services? Nothing in
juju allows the local unit to fiddle with relation names and ids from
remote services or units.

https://codereview.appspot.com/5837050/

Revision history for this message
Jim Baker (jimbaker) wrote :

https://codereview.appspot.com/5837050/diff/3001/source/drafts/relation-info.rst
File source/drafts/relation-info.rst (right):

https://codereview.appspot.com/5837050/diff/3001/source/drafts/relation-info.rst#newcode7
source/drafts/relation-info.rst:7: relation-info [--interface INTERFACE]
[-r RELATION_NAME_OR_ID]
This is a nice simplification. What I originally proposed was to use a
parallel arg/option structure to the other relation hook commands. But
this works much better.

https://codereview.appspot.com/5837050/diff/3001/source/drafts/relation-info.rst#newcode21
source/drafts/relation-info.rst:21: possible to filter that as desired
without using the `--interface`
This part of the proposal was out of date. The original proposed format
for the relation id used <interface>-<normalized internal relation id>.
Now it's <relation name>:<normalized internal relation id>. So we may
still want to support --interface, per the Keystone example in this
proposal.

https://codereview.appspot.com/5837050/diff/3001/source/drafts/relation-info.rst#newcode25
source/drafts/relation-info.rst:25: verify the continued existence of
this relation id for the
Sounds good to me.

https://codereview.appspot.com/5837050/diff/3001/source/drafts/relation-info.rst#newcode31
source/drafts/relation-info.rst:31: script. Calling `relation-info` for
other services is read at that
Fair enough.

https://codereview.appspot.com/5837050/

8. By Jim Baker

Now relation-ids and corresponding simplification

Revision history for this message
Jim Baker (jimbaker) wrote :
Revision history for this message
Jim Baker (jimbaker) wrote :

https://codereview.appspot.com/5837050/diff/3001/source/drafts/relation-info.rst
File source/drafts/relation-info.rst (right):

https://codereview.appspot.com/5837050/diff/3001/source/drafts/relation-info.rst#newcode7
source/drafts/relation-info.rst:7: relation-info [--interface INTERFACE]
[-r RELATION_NAME_OR_ID]
On 2012/03/20 19:47:36, niemeyer wrote:
> If this command enumerates relation ids, how can it accept a relation
id as a
> parameter? Also, I don't see how it can take a service name as a
parameter
> either, or why it takes an interface, or even a unit name.

> If I understand what this command is doing, I'd imagine something much
simpler
> instead:

> relation-ids [<relation name>]

> This will list the given relation name, or the relation name currently
executing
> if the parameter is omitted.

> If you have real problems that need a more complex interface than
this, can you
> please ping me online? I'd be happy to understand them.

Done.

https://codereview.appspot.com/5837050/diff/3001/source/drafts/relation-info.rst#newcode21
source/drafts/relation-info.rst:21: possible to filter that as desired
without using the `--interface`
Simply removed interface discussion, since no real use case, per irc
discussion.

https://codereview.appspot.com/5837050/diff/3001/source/drafts/relation-info.rst#newcode25
source/drafts/relation-info.rst:25: verify the continued existence of
this relation id for the
On 2012/03/20 20:00:47, jimbaker wrote:
> Sounds good to me.

Done.

https://codereview.appspot.com/5837050/diff/3001/source/drafts/relation-info.rst#newcode31
source/drafts/relation-info.rst:31: script. Calling `relation-info` for
other services is read at that
On 2012/03/20 20:00:47, jimbaker wrote:
> Fair enough.

Done.

https://codereview.appspot.com/5837050/

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

LGTM, some minors.

https://codereview.appspot.com/5837050/diff/9001/source/drafts/relation-info.rst
File source/drafts/relation-info.rst (right):

https://codereview.appspot.com/5837050/diff/9001/source/drafts/relation-info.rst#newcode16
source/drafts/relation-info.rst:16: For any hook, the data for the
`relation-ids` command is cached prior
This feels like an internal impl detail, i suggest its removal.

https://codereview.appspot.com/5837050/diff/9001/source/drafts/relation-info.rst#newcode36
source/drafts/relation-info.rst:36: list of relation ids that provide
the keystone interface, then
"of the relation ids for the relation instances with the
keystone-service name and keystone service interface."

The comma here should be a period. The relation instances provide the
interface.

https://codereview.appspot.com/5837050/diff/9001/source/drafts/relation-info.rst#newcode37
source/drafts/relation-info.rst:37: enumerated over to set the ready for
each relation. Assume that
This part after the comma doesn't make any sense to me, i'd suggest its
removal.

"then enumerated over to set the ready for each relation"

https://codereview.appspot.com/5837050/

Revision history for this message
Mark Mims (mark-mims) wrote :

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches