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

[JENKINS-11726] Do not count NOT_BUILT. #7

Merged
merged 2 commits into from Aug 29, 2012

Conversation

Projects
None yet
4 participants
@jorgenpt
Contributor

jorgenpt commented Nov 15, 2011

Do not count NOT_BUILT against total build value, nor count it as a non-success.

This is required for pathignore-plugin to "skip" some gerrit verifiers: https://github.com/jenkinsci/pathignore-plugin

The pathignore plugin is very helpful for having multiple verifiers for one repository, but not running all of them if only paths of the repository is changed. :-)

@@ -309,6 +309,9 @@ protected int getVerifiedValue(Result res, GerritTrigger trigger) {
protected int getMinimumVerifiedValue(MemoryImprint memoryImprint) {
int verified = Integer.MAX_VALUE;
for (Entry entry : memoryImprint.getEntries()) {
if (entry.getBuild().getResult() == Result.NOT_BUILT) {
continue;

This comment has been minimized.

@rsandell

rsandell Jan 9, 2012

Member

What if all builds where NOT_BUILT, I think that could cause strange behaviour?

This comment has been minimized.

@jorgenpt

jorgenpt Jan 10, 2012

Contributor

I think that just behaves as if there were no jobs run for it, doesn't it?

On Mon, Jan 9, 2012 at 7:25 AM, Robert Sandell <
reply@reply.github.com

wrote:

@@ -309,6 +309,9 @@ protected int getVerifiedValue(Result res,
GerritTrigger trigger) {
protected int getMinimumVerifiedValue(MemoryImprint memoryImprint) {
int verified = Integer.MAX_VALUE;
for (Entry entry : memoryImprint.getEntries()) {

  •        if (entry.getBuild().getResult() == Result.NOT_BUILT) {
    
  •          continue;
    

What if all builds where NOT_BUILT, I think that could cause strange
behaviour?


Reply to this email directly or view it on GitHub:
https://github.com/jenkinsci/gerrit-trigger-plugin/pull/7/files#r336615

This comment has been minimized.

@rsandell

rsandell Jan 12, 2012

Member

This particular method would return Integer.MAX_VALUE if all builds are NOT_BUILT

I am unsure what impact that would give on the rest of the functionality.

This comment has been minimized.

@jorgenpt

jorgenpt Jan 12, 2012

Contributor

Hm, yeah, I thought I had run that test scenario, but it seems (by tracing code) like the behavior would be that we'd call gerrit approve --verified MAX_INT.
What do you think we should do in this scenario? Should we set verified = 0, and maybe have a "Build Not Run" message instead of "Build Successful"?

This comment has been minimized.

@rsandell

rsandell Jan 30, 2012

Member

I have been thinking about that for a while and can't really come up with the best solution for how it should behave.
I guess that a build started has been sent to Gerrit for the builds (has it?) If that is the case then some kind of response should be reported back, otherwise the users will think that something is wrong, so a "nothing was run" message is probably the best thing to provide.

This comment has been minimized.

@jorgenpt

jorgenpt Jan 31, 2012

Contributor

I don't think anything is sent before the "build complete" message. At least on our instance, nothing shows up until the build completes.

There might be an option to make it post a "build started" note, though.

This comment has been minimized.

@bnovc

bnovc Jun 19, 2012

You can configure which messages are sent (started, completed). You may have removed the started message on your instance.

It seems like a good idea to add a configurable message for "Nothing built" and have it send that in this case.

DRY some of the tests.
The GerritTriggerTest has a large amount of identical code. This tries to DRY
that code a little bit via refactoring.

This also DRY-es up a bit in the ParameterExpanderTests.
@jorgenpt

This comment has been minimized.

Contributor

jorgenpt commented Aug 2, 2012

This change is now a little bit more substantial, and I've clearly defined how it should work.

  • If there's one or more completed builds (unstable, successful or failed), NOT_BUILT builds are ignored.
  • If there're no completed builds (all builds are NOT_BUILT), then we use the new configuration variables for code review, verified and gerrit command to notify the service.

I've also cleaned up some of the tests and added some testing for NOT_BUILT.

@buildhive

This comment has been minimized.

buildhive commented Aug 2, 2012

Jenkins » gerrit-trigger-plugin #27 SUCCESS
This pull request looks good
(what's this?)

[JENKINS-11726] Do not count NOT_BUILT as a fail.
With this change, NOT_BUILT changes are ignored when tallying up what code
review / verified score to give a change. If none of the builds complete
(they're all "NOT_BUILT"), then we use the score & command assigned to the
NotBuilt event.

This is required for plugins to be able to "skip" some gerrit verifiers. (like
the pathignore-plugin)
@buildhive

This comment has been minimized.

buildhive commented Aug 3, 2012

Jenkins » gerrit-trigger-plugin #28 SUCCESS
This pull request looks good
(what's this?)

@rsandell

This comment has been minimized.

Member

rsandell commented Aug 4, 2012

OK, I'll take a deeper look when I'm back from my vacation in about a week.

rsandell added a commit that referenced this pull request Aug 29, 2012

Merge pull request #7 from jorgenpt/dont-count-not_built
[JENKINS-11726] Do not count NOT_BUILT.

@rsandell rsandell merged commit 7725166 into jenkinsci:master Aug 29, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment