btkelly / gnag

A Gradle plugin that helps facilitate GitHub PR checking and automatic commenting of violations.
http://gnag.watch
Apache License 2.0
125 stars 14 forks source link

Run different tools in parallel #210

Closed szaboa closed 5 years ago

szaboa commented 5 years ago

@stkent Hi, so what I've done so far:

Separated the checks by different tools in new tasks, so they are able to run parallel on a CI.

However, I have problem with two specific tasks (GnagKtlintTaskand GnagDetektTask), the others are working fine if you run them separately.

As you can see I created a base class for these tasks (BaseGnagCheckTask), and it would require to cast the two mentioned tasks to this base, however these have JavaExec.class as type so casting is not really working here.

As I am a newbie in writing Gradle plugins, I wanted to ask if this is can be solved somehow or not? The casting more specifically is here:

Why I need to cast these? Because I would need to call these:

gnagCheckTask.setGnagPluginExtension(gnagPluginExtension);
gnagCheckTask.resolve(project);

The first one sets the plugin extension, so the base will have it and can use it (e.g. gnagPluginExtension.shouldFailOnError() inside com.btkelly.gnag.tasks.BaseGnagCheckTask#executeGnagCheck).

The second one will call the resolve method, which will populate the violationDetectors array,


We could get rid of the resolve and the whole ViolationResolver interface, however the base still needs to get the gnagPluginExtension somehow.

Do you have an idea how could we solve this nicely? :)

Note. I didn't touch the reporter task yet.

szaboa commented 5 years ago

@stkent Ok, so looked into these tasks and as it turned out these behave like any other classes so my first question was silly. I ended up to wrap these JavaExectasks (for ktlint and detekt) into GnagKtlintTaskand GnagDetektTaskby simply adding as a dependency.

So all the tasks are separated without any code duplication, they also can be run together with GnagCheckTask.

Please have a look to this new architecture and let me know if something needs to be adjusted. Note. I haven't modified the report task yet, but I will - to be able to report single tasks as well.

Cheers.

szaboa commented 5 years ago

@stkent @btkelly Added a runCheck parameter, which can be used to avoid depending automatically on check task inside the report task.

From now the check tasks can run parallel, because they are separated. Each of them will write out the found violations and the report task will read these. If this is the use case, then the runCheck should be configured to false, to avoid running the bulk check task again.

Note. I will rename runCheck to runCheckOnReport.

Please have a look on the changes, if you don't have time to do this, it's OK if I publish my version to jcenter?

btkelly commented 5 years ago

@szaboa I'm really sorry for taking so long on this, things have been busy as of late. And unfortunately I will not be able to get to this until probably week after next. I appreciate the PR but with the size of it I want to take a deeper dive into the changes before merging. One thing off the bat that I would like to steer away from is exposing all of the sub tasks to the user as the idea behind Gnag was to abstract all of the other tools. Not necessarily a deal breaker but I have to look a bit more at the PR. Again thanks for the contributions, always appreciated!

szaboa commented 5 years ago

Sure, no hurry. Regarding exposing the subtasks, I don't see why this would be a wrong thing, users still able to run the master gnag check (which includes everything), and giving the possibility to run them separate is advantage, as it is more flexible (e.g. can be executed in parallel), at least for me. This is just my opinion, but I will accept however you decide with respect :)

btkelly commented 5 years ago

Closing this for now. I do like the idea of allowing parallel tasks but I think a larger refactor is needed if we want to expose everything as their own tasks. I think if we register custom tasks they should extend a common task or something along those lines and remove the idea of the detector. At the very least bring the two together so if a detector is disabled the task does not show.