search

apache / fluo

Apache Fluo
https://fluo.apache.org
Apache License 2.0
187 stars 78 forks source link

Command module with centralized Command Pattern logic #1083

Closed jkosh44 closed 4 years ago

jkosh44 commented 4 years ago

This is my first pass attempt at a POC for using the command pattern to implement the Fluo command module. It still needs some polishing up and I want to add tests but I wanted to give people a chance to review and comment before continuing on. It doesn't conform to the command pattern 100% but it's pretty close, and I found forcing it to conform 100% was either making it too verbose or unnecessarily complicated.

This ended up touching just about every file in the command module and became a decently big change.

I didn't attempt to solve the System.out/System.err vs Logger problem as I described in Issue #983 but it may be something to consider while reviewing.

The overall goal of this PR is to make it easier to reuse the commands outside of the command line (in a UI or API for example) while moving the logic to run them from the command line to a central location.

Fixes #983

jkosh44 commented 4 years ago

So I'm pretty happy with the current status of this PR and I think I've addressed all the comments. If I've missed any or if there are other issues please let me know. I'm going to work on adding tests and doing some functional testing. It might take a little while because I need to figure out the best way to test all the commands.

There's a couple of potential follow up issues though that I've come across

  1. Investigate possible ways of condensing the fluo script. After this change the fluo script has become pretty repetitive, there may be some ways of condensing it.
  2. Figure out solution for printing to logger vs System.out. Some commands print to System.out while others log to a Logger
  3. Make commands more friendly to be used outside of CLI. (This may be very related to number 2). Because execute returns void, the caller doesn't get much information from the commands, just whether it was successful or if there was an Exception. For example FluoList is pretty useless outside of the CLI right now, it just prints the running applications to System.out. Issues like #670 could benefit from being able to use the info from FluoList. One possible solution would be to move the functionality to another method like public T list() where T was some type that had the relevant information for listing applications. Then execute() could call list() and print the information to System.out. The web app mentioned in #670 could just call list() directly. The other commands would each have to be looked at individually.
keith-turner commented 4 years ago

So I'm pretty happy with the current status of this PR and I think I've addressed all the comments.

I think this is in really good shape. I like the recent change making command names an annotation parameter.

Investigate possible ways of condensing the fluo script.

Could possibly open a follow on issue and advertise that on Apache helpwanted as a bash task. I like narrow and very specific issues for helpwanted, and this feels like one.

Figure out solution for printing to logger vs System.out.

Could open up a follow on issue for that.

jkosh44 commented 4 years ago

Thanks for the help reviewing @keith-turner. I'm hoping to finish up the tests sometime after the holidays, probably in the new year. I can open up those issues after a final review and merge. I like the idea of putting the bash issue on help wanted. The other issue I'll probably just grab myself unless someone else is particularly interested.

keith-turner commented 4 years ago

@jkosh44 what testing have you done for this? I plan to pull this PR down locally and try running it. Before I did that I was curious what testing you had done. Also, are there any significant outstanding changes you still plan to make for this PR? If so, I will hold of on testing it locally.

jkosh44 commented 4 years ago

@keith-turner after last night's change I have no more planned changes for the actual functionality. I do still want to try and add Integration Tests. I'm not sure how I'm going to do that right now or if it's possible so I plan on looking into that next.

In terms of testing I used uno and stresso to compare running the commands of the previous version to running the commands on my version. There's still probably some edge cases and optional parameters to test but I'm going to do that after integration tests.

So feel free to pull this down and test it yourself. I don't plan on changing any of the main code unless an issue is found.

I also want to squash all the commits once it's fully complete.

keith-turner commented 4 years ago

I also want to squash all the commits once it's fully complete.

That is something usually do after review is complete and its approved. I usually use the Squash and Merge button on github (there is a drop down arrow beside the Create a Merge Commit button) when there are no merge conflcts. Be careful with the Squash and merge button though. In the past it failed for me and when it did a retry button showed up. When I pressed retry it merged all of my commits instead of doing squash and merge. This has happened to me twice, so if I see the it in the future I am not going to retry.

jkosh44 commented 4 years ago

@keith-turner Unless you think we should do integration tests as a follow up PR since this has grown fairly big already. In that case we can just do functional testing, squash, final review, and merge.

Edit: Sorry just saw your comment above. Yeah, squash and merge method sounds good.

keith-turner commented 4 years ago

@jkosh44 I think integration test as a follow on issue is a good way to go. I will run this locally today or tomorrow and after that I will be finished reviewing.

jkosh44 commented 4 years ago

@keith-turner Alright all the comments have been addressed. Is it ok to merge my own PR or should I let someone else do it?