fabric8-services / fabric8-jenkins-idler

OpenShift.io service to idle resp.unidle Jenkins instances
Apache License 2.0
4 stars 15 forks source link

Create make dev target for running idler locally #138

Closed kishansagathiya closed 6 years ago

kishansagathiya commented 6 years ago

Trying to run idler locally is very painful. It requires us to run 4 other services and set up quite a few environment variables. We would not want to go through this pain everytime we run idler. Instead, we can automate this by adding dev tag in our Makefile, in which we would run all these services through docker and set up environment variables as well. Alternatives to Makefile were suggested by @sthaha such as godo, grift

jaseemabid commented 6 years ago

@kishansagathiya I agree with you in the goals of the task 100%, but let's not introduce external tools for this, there is nothing here that needs more than a few lines of bash.

jaseemabid commented 6 years ago

This is related. https://github.com/fabric8-services/fabric8-jenkins-idler/pull/135

jaseemabid commented 6 years ago

Duplicate of https://github.com/fabric8-services/fabric8-jenkins-idler/issues/128

hferentschik commented 6 years ago

Trying to run idler locally is very painful. It requires us to run 4 other services and set up quite a few environment variables.

So why don't you use the script setupLocalIdler.sh? This will take care of everything. That said, the need to run Idler fully integrated should be minimal.

We would not want to go through this pain everytime we run idler.

That's why the script exist to make it easy to run against some actual data. The script will also

Instead, we can automate this by adding dev tag in our Makefile, in which we would run all these services through docker and set up environment variables as well.

So that I understand correctly, your plan is to create and configure all of the dependent services for Idler? This is ways more complicated that what setupLocalIdler.sh does and it does not even address the issue of data. You would have to seed the auth service, you would still need a way to monitor the OpenShift cluster for events, ...

I am not saying this is not possible, but the amount of effort requires is substantial. I also doubt that you should "pack" this into the Makefile. You will need a considerably amount of plumbing. Make is not the way to do that. Also, you are talking about a single target. You would most probably need more. One to start and stop the service and one to configure your shell (the equivalent to `setupLocalIdler.sh env).

Persoanlly I think this is not the right way to do it. If you want to go down this path, however, you should create a script to handle all this. If you want to integrate this into the Makefile, then the Makefile targets should just call the scripts with the right parameters.

Alternatives to Makefile were suggested by @sthaha such as godo, grift

-1, I don't think it is a good idea to introduce these dependencies. A bash script probably does the job. No need to require and install additonal tools.

hferentschik commented 6 years ago

Duplicate of #128

How is this a duplicate of #128?

kishansagathiya commented 6 years ago

@hferentschik

then the Makefile targets should just call the scripts with the right parameters.

That was the plan. But if setupLocalIdler.sh does the job, we won't need to go through the proposed path. Just to confirm it once, the script (setupLocalIdler.sh) runs all the required services. It doesn't seem to be setting up environment variables, does it? At least I was getting the same error ( that the environment variables are not setup) I am yet to get my hands on the entire mail thread regarding this one. Probably after that, I would have a better idea about how to do the local setup.

kishansagathiya commented 6 years ago

@jaseemabid #128 is about documentation of the current way of running idler locally. This one is about a different approach to run idler locally and exploring possibilities of doing so very simply say in one command like make dev.

kishansagathiya commented 6 years ago

@hferentschik

That said, the need to run Idler fully integrated should be minimal.

Can you elaborate on this one? If by fully integrated you mean along with all these services, running the idler results in an error if one of this env variable is not set. Am I missing something here?

hferentschik commented 6 years ago

Can you elaborate on this one?

Sure

If by fully integrated you mean along with all these services, running the idler results in an error if one of this env variable is not set. Am I missing something here?

It depends. So yes, you need to have all the dependencies of the Idler fulfilled in order to run it. And there will imo always be cases where one wants to run the Idler locally, hence setupLocalIdler.sh. There are other ways to do this - Docker, Minishift,... However, all of these come with a considerable overhead to set them up and to maintain the setup. Not even speaking about that they put an unnecessary load on your machine.

That said, the aim should be to as much as possible just use plain unit tests to define and test a certain piece of functionality. You define what your new/changed behaviour should be and then you write a unit test for it. This requires much more coding against interfaces and breaking things down in smaller pieces. One big part of the last couple of months of development was to get Idler (and Proxy) into a state where this is possible. We are not quite there yet, but we are getting close. For example, for the latest Idler bug in relation of backing of idle calls, I could write unit tests to verify this particular functionality. The unit test can be run over and over again and it proves that a certain condition is met. If you were to just implement this and manually verifying it, there would be no repeatable way to assert this behaviour, no a guarantee that you are regressing.

I'd argue that 90% (if not more) of required changes can be developed and tested without running the full Idler. Running it locally should be to sanity check something or collect data in order to develop a test. So it should be the exception not the norm.

Again, we are not there yet. There are several rough edges still in the code. However, instead of trying to fix these things by running the Idler locally, the aim should be to further decompose these problematic pieces of code, add unit tests for the smaller pieces.

Does this answer your question?

kishansagathiya commented 6 years ago

@hferentschik Yes. That clears a lot of things. thanks a lot.

hferentschik commented 6 years ago

@kishansagathiya what do you think, can we close this issue?