Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not return != 0 unless there are errors. #71

Closed
juliogonzalez opened this issue Oct 3, 2017 · 7 comments
Closed

Do not return != 0 unless there are errors. #71

juliogonzalez opened this issue Oct 3, 2017 · 7 comments

Comments

@juliogonzalez
Copy link
Member

After #47 there seems to be a problem.

When there are no PRs we are now returning 0, which is correct since there were no errors.

However Jenkins does the following:

  • If gitarro returned 0, it changes it to 1, so downstream job is not launched.
  • If gitarro returned 1, it changes it to 0, so downstream job is launched.

We ned to change this behaviour. A Jenkins job must only fail when there is a problem, and not having PRs with changes is not a failure.

What I propose here is to keeping retur 0 when there are no PRs, and modify the output to include a text that we can easily identify with Jenkins.

Right now the text is:
There are no Pull Requests opened or with changes newer than 3600 seconds

Let's change it to:

[NOPRS] There are no Pull Requests opened or with changes newer than 3600 seconds

When --changed_since >= 0

And

[NOPRS] There are no Pull Requests opened

When --change_since < 0 (in fact this is small bug, since it didn't crashed but showed "or with changes newer than -1 seconds")

So we can easily grep if return code is 0 and determine if there were PRs or not without breaking the jobs (in fact we should configure them to send email when they REALLY break, for example because gitarro is not working -a bug, permission problems, etc...).

In the meantime I will remove --changed_since from the log.

With the refactor we should also check if we are doing this at other places.

@MalloZup
Copy link
Contributor

MalloZup commented Oct 3, 2017

Ok for me, i would only vote instead for the text, a bool variable. ( that jenkins can grep).
If we want to move away from jenkins, we need some general behaviour.

Indeed this behaviour was introduced the time we use the -C option, for running downstram jobs.
This complicate a bit the stuff/behaviour of gitarro.

I think we should add some spec_tests for catching this, meaning:

  • add test for ```-changed-since`` with the -C option, and see if we return the bool var.

We have also to consider the 2 way to run gitarro : scheduler -C and runner.

When we execute as Runner, imho is totally ok to have a Red Ball (-1) when a test fail. For unexpected errors there is the yellow ball in jenkins.

The only stuff is that the additional variable, wiil add a extra=dependencie to how the jenkins jobs are configured. ( grepping for a variable)

@juliogonzalez
Copy link
Member Author

Sounds great for me as well.

Do you mean something like:

[PRS=FALSE] There are no Pull Requests opened

?

@MalloZup
Copy link
Contributor

MalloZup commented Oct 4, 2017

fixed by #77.

@MalloZup MalloZup closed this as completed Oct 4, 2017
@juliogonzalez
Copy link
Member Author

Reopening.

We are still returning != 0 at some other places such as lib/gitarro/backend.rb line 147

@juliogonzalez juliogonzalez reopened this Oct 5, 2017
@MalloZup
Copy link
Contributor

MalloZup commented Oct 5, 2017

right :)

@MalloZup
Copy link
Contributor

MalloZup commented Oct 9, 2017

there is only 1 in backend which is correct to have it there we can close this

@MalloZup
Copy link
Contributor

MalloZup commented Oct 9, 2017

closubg feel free to reopen, but the only 1 place is not enough :) thx
The place there is right for exiting 1 after test fail

@MalloZup MalloZup closed this as completed Oct 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants