On 02/07/12 11:03 -0400, Mo Morsi wrote:
On 07/02/2012 10:38 AM, Jason Guiditta wrote:
> On 29/06/12 14:06 +0530, Samridh wrote:
>> This patch adds hwp creation, deletion and listing functionality to
>> the cli.
>>
>> To test the functionality, please modify the :url entry in your
>> ~/.aeolus-cli file to something like ":url:
http://<host>/conductor"
>> i.e, remove the /api from the :url entry. This is because all the
>> conductor side funcitonality has been implemented by modifying the
>> hardware_profile cotroller itself. You could also modify the
>> routes.rb on the conductor side, but I personally found the former
>> easier to do.
>
Jay, appreciate the feedback
> I'll try to reply in more detail later, but this is not going to be
> able to go in as is. I'll list a few of the large problems here, and
> get into details later or tomorrow once I finish some other things on
> my todo list:
> * You can't break the entrypoint in the configu file like this, it
> will cause the existing stuff that _does_ require /api in the uri to
> break
We had previously discussed that the /api controller namespace would be
merged into the top level controller layer for conductor, leveraging the
same methods the web interface uses, but simply returning responses in
XML format.
Since all new additions will not leverage the /api namespace, I feel it
would make more sense to continue forward in this direction, and not
incorporate '/api' into the cli entry point. The controllers currently
in the api namespace will need to be integrated into the top level
controller layer in conductor in whatever case.
This is correct, but new functionality cannot break existing stuff,
and that will not be changing until the image management engine is
pulled in. That said, if this works as is with the url change in
confog file, I could live with saying it just doesnt work by default
until the above-mentioned change.
> * hwp is _not_ a bolton to aeolus-image, it belongs in a separate
> command (this could be just hwp or something else, not sure
> offhand). Calling aeolus-image --hwp makes no sense, as an hwp is
> not an image.
Weren't we going to rename the 'aeolus-image' command to be
'aeolus-cli'
so that it is more applicable to general purpose conductor management. I
thought this was the point of incorporating the subcommands into the cli
utilities, with the end goal being to be able to manage all the
conductor entities from the command line.
No, this is not exactly the design. There is ann 'aeolus' wrapper
script (option parser) that outputs general help and flags that are
common to all commands. It then invokes the appropriate
parser for handling the specific command that was invoked. In the
current codebase, the default is aeolus-image, and we decided that
since it was the thing the cli tooling was being most used for at the
time, calling 'aeolus list' (for example), would just be a shortcut for
either:
* aeolus image list
* aeolus-image list
So, assuming HWP does not itself need to be encapsulated in some more
general 'category of work', the extension of the pattern woudl be
something like:
* aeolus hwp list
* aeolus-hwp list
I just took a quick look at the existing code, and really what we
should also be doing is extending, say ListCommand with a
HwpListCommand, which only has the actions specific to that object
type. However, I can see that complicating this task a bit, so it can
be a later refactor, as it would have no api implication, just cleaner
code behind the api.
-j
> * where are the tests? we cannot accept new functionality without
> tests.
Agree on this one, the new additions could use some specs and similar to
verify functionality. Should be straightforward to add as there are
plenty of examples in place of how to do things.
-Mo