Fixes lp#1628919: help-tool fix for payload hook tools. #7515
Conversation
…validity of hook context.
cmd/juju/commands/helptool_test.go
Outdated
| // Component-based features such as payloads and resources | ||
| // are different enough in implementation to the rest | ||
| // of Juju code that we need to ensure that help-tool can reach them | ||
| // explicitely. |
There was a problem hiding this comment.
That said, we already do something like this for 'juju commands' where we have a list of commands that we expect to see registered and available.
Would it be reasonable to just list all commands here? It does need updating whenever we add/remove a command, but it also means we don't remove a registration without noticing.
There was a problem hiding this comment.
I was wondering that. I'll change and will hard code the list.It'll also reduce number of tests back down :D
payload/context/register_test.go
Outdated
| } | ||
|
|
||
| func (s registerSuite) TestInitNilArgs(c *gc.C) { | ||
| err := s.command.Init(nil) |
There was a problem hiding this comment.
All test functions need to be '*' in their receivere.
(s *registerSuite)
Otherwise it does bad things with base classes like IsolationSuite (we track things in AddCleanup that do bad things if you use a value rather than a pointer)
There are several places where this needs to be fixed.
Are there any changes here that aren't just pulling 's.command' into SetUp() (and changing hook context into an acquisition function)? I didn't see any.
There was a problem hiding this comment.
I'll change the receivers but no, all the changes here are related to how the command is constructed for the tests and how the tests get it.
| return errors.Trace(err) | ||
| } | ||
|
|
||
| // TODO(ericsnow) Is the flush really necessary? | ||
|
|
||
| // We flush to state immedeiately so that status reflects the | ||
| // We flush to state immediately so that status reflects the |
There was a problem hiding this comment.
hm, I wonder if this was copy & pasted code. :)
There was a problem hiding this comment.
No :) I am sure the same word is mistyped 3 times in the same way in the similar-ish looking code :D
|
So the primary changes are that payload hook command are registered (the call to registerHookContextCommands) and that constructors for these commands do not prevent construction if given hook context is malformed/non-functional. |
…ed commands. Changed payload register test suite to have pointer receivers.
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
This change may require documentation update. |
|
@anastasiamac Nothing to do here! |
Description of change
'juju help-tool' did not contain payload hook tools.
Payload hook tool commands are the only ones in juju codebase that required a valid hook context in the constructors. This approach, whilst commendable, does not work for help-tool command which uses mock dummy hook context to get help information from all commands it can see.
In fact, valid hook context is only important at the time when it is used, in the command's Run method. Majority of this PR changes payload hook commands to not error at construction time.
Also, since payload hook tools are implemented under component architecture, these hook tools needed to be explicitly registered to be in collection of all hook tools.
Added unit test to ensure that component based hook tools are displayed as part of help-tool output.
QA steps
Output of 'juju help-tool' should contain:
payload-register register a charm payload with juju
payload-status-set update the status of a payload
payload-unregister stop tracking a payload
Documentation changes
If output of 'juju help-tool' is shown in online documentation, it may need to be adjusted.
Bug reference
https://bugs.launchpad.net/juju/+bug/1628919