|
|
|
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)
Patch Set 1 #Patch Set 2 : Add `relation-info` relation hook command. #
Total comments: 12
Patch Set 3 : Add `relation-info` relation hook command. #
Total comments: 3
Total messages: 7
|
jimbaker
Please take a look.
|
13 years, 10 months ago (2012年03月15日 19:44:23 UTC) #1 | ||||||||||||||||||||||||
Please take a look.
Please take a look.
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.... 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.... 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.... 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.... 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/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.... 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.... 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.... 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.... source/drafts/relation-info.rst:31: script. Calling `relation-info` for other services is read at that Fair enough.
Please take a look.
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.... 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.... 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.... 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.... 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.
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.... 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.... 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.... 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"