search

ansibleplaybookbundle / ansible-playbook-bundle

THIS REPO IS MIGRATING: https://github.com/automationbroker/apb
GNU General Public License v2.0
140 stars 70 forks source link

Proposal: apb tool refactor #197

Closed rthallisey closed 6 years ago

mhrivnak commented 6 years ago

One aspect that adds substantial noise and overhead to the "engine" today is printing output. I suggest that would be best paired with the other CLI parts. The actions themselves, currently called "commands", would be more like a library that just gets stuff done.

cli.go (or cli.py) would have all of the Subcommand features you listed. It would parse+validate input, determine the correct function to call that will perform real work, and call it with the right arguments. Additionally I'm suggesting that it would receive a return value from that function, and then be fully responsible for all output to stdout.

This would isolate output serialization decisions, such as yaml or json or pretty-print or other, which fields to include, etc. to the same part of code that handles input. For example:

cli.go would have a subcommand for apb list. It would parse and validate arguments somehow. It would then call the correct function in commands/broker_requests/apb_list.go. The return value would be a collection of results in native golang structures. Then the subcommand would serialize those results in the appropriate way, based on user input (-o something), and print them to stdout.

Thoughts on that?

If we went that direction, we might then consider having a commands package that holds everything cli.go has plus whatever serialization code there is, and a separate actions or lib package (not sure of the best name) that contains the remaining "engine" code (of course organized into classes as proposed).

rthallisey commented 6 years ago

@mhrivnak that sounds good to me. I'll make sure your comment is reflected in the proposal.

shawn-hurley commented 6 years ago

I would like to suggest that use the cobra library for developing the commands in go.

eriknelson commented 6 years ago

We've been having a lot of conversations in the broker project about breaking the current repo into 3 projects. The relevant one would be a libapb, which would directly affect this effort. Fortunately, I think it should make this easier. The basic idea is that the bulk of the actual APB logic, data structures, validation, execution etc., will be vendorable via libapb. It is the authoritative project for all things APB. Anyone working with APBs can standardize on top of this library and benefit from upstream feature development and maintenance. It also means libapb consumers become lean layers on top of it, only containing their particular concerns. apb becomes basically just a cli wrapper for driving libapb.

eriknelson commented 6 years ago

It would be interesting to continue the discussion around this in light of the past month's work. Since my last comment, APB should be replaced with "Service Bundle", and we actually have a vendorable bundle-lib that is effectively what I described as libapb thanks to @shawn-hurley's work.

jmrodri commented 6 years ago

@shawn-hurley @eriknelson is cobra really the best CLI parsing for golang? When I used it for my sm-photo-tool project I wasn't a fan. I found it intrusive. But it's been a year since I last played with it.

eriknelson commented 6 years ago

@jmrodri it feels like the community's default. I don't think that's enough to warrant a blank pass, but there's a lot of precedence and value in using what everybody else is using. I don't have enough experience with it to assert an opinion. I'll give it a try this weekend and weigh-in.

shawn-hurley commented 6 years ago

@jmrodri I would like to hear how it was intrusive?I agree with @eriknelson tt does seem to be the standard that most people are using.

dymurray commented 6 years ago

@rthallisey do we want to merge this in and continue work on the google doc/second PR we have open?

rthallisey commented 6 years ago

@dymurray I forgot about this PR. I'll merge the relevant bits with https://github.com/ansibleplaybookbundle/ansible-playbook-bundle/pull/279. Then we can continue updating the google doc and I'll pull it all together for one final product.

rthallisey commented 6 years ago

work has already begun here: https://github.com/automationbroker/sbcli