Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-30393

Don't vote 0 if voting is skipped

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      We would like to be able to configure that a job reports a status message (thus it cannot be configured as silent) but doesn't vote. The current behavior is that if all jobs triggered together are marked as "skipped" that the plugin votes with 0. Instead we would like that it doesn't vote at all and thus doesn't overwrite previous votes.

      What do we need to do to be able to get this into the main version (we will try to implement it ourselves)?
      Are you open to change this behavior? I think that skip voting if all are marked as "skipped" would be the more sensible default. Or would this require a new option? If so how would you like this to be configured?

        Attachments

          Issue Links

            Activity

            Hide
            rschulz Roland Schulz added a comment -

            Implementation wise I suggest:

            • getMinimumCodeReviewValue and getMinimumVerifiedValue return Integer.MAX_VALUE instead of 0 if all are skipped
            • BuildCompletedRestCommandJob.createReview() checks for MAX_VALUE and skips voting
            • For non-REST one either keeps the old behavior (by checking for MAX_VALUE and replace with 0) or one needs to remove the argument from the string in ParameterExpander.getBuildCompletedCommand
            Show
            rschulz Roland Schulz added a comment - Implementation wise I suggest: getMinimumCodeReviewValue and getMinimumVerifiedValue return Integer.MAX_VALUE instead of 0 if all are skipped BuildCompletedRestCommandJob.createReview() checks for MAX_VALUE and skips voting For non-REST one either keeps the old behavior (by checking for MAX_VALUE and replace with 0) or one needs to remove the argument from the string in ParameterExpander.getBuildCompletedCommand
            Hide
            rsandell rsandell added a comment -

            Well this is a bit tricky to achive. My original goal with the implementation was that it should just put a comment. But since the ssh review commands are templatized in a bit of a "hard coded" manner, in that the template contains "--code-review <CR> --verrified <VRIF>" the only way I saw to fix it was to provide a value anyways.

            Now I would like to make the approval values fully configurable (as in not being specific to Code Review and Verified) so the admin can provide what labels are available to set for any given server.
            That feature would then make it needed to change the ssh command templates to be more dynamic (not containing --code-review <CR> et.al. but more like <LABEL_VALUES> for example). And then we could omit the approvals all together if all builds should skip voting as a bonus.

            But that is a big endeavour to implement.

            Show
            rsandell rsandell added a comment - Well this is a bit tricky to achive. My original goal with the implementation was that it should just put a comment. But since the ssh review commands are templatized in a bit of a "hard coded" manner, in that the template contains "--code-review <CR> --verrified <VRIF>" the only way I saw to fix it was to provide a value anyways. Now I would like to make the approval values fully configurable (as in not being specific to Code Review and Verified) so the admin can provide what labels are available to set for any given server. That feature would then make it needed to change the ssh command templates to be more dynamic (not containing --code-review <CR> et.al. but more like <LABEL_VALUES> for example). And then we could omit the approvals all together if all builds should skip voting as a bonus. But that is a big endeavour to implement.
            Hide
            rschulz Roland Schulz added a comment -

            Yes for non-REST one has indeed the difficulty you pointed out. But for REST it is much easier. Thus one could change the behavior for REST only, but then the behavior is different for REST and non-REST. The other option would be to remove the word before the <CR> tag from the template. Of course it isn't as nice as your more complete solution but would also be easier.

            Show
            rschulz Roland Schulz added a comment - Yes for non-REST one has indeed the difficulty you pointed out. But for REST it is much easier. Thus one could change the behavior for REST only, but then the behavior is different for REST and non-REST. The other option would be to remove the word before the <CR> tag from the template. Of course it isn't as nice as your more complete solution but would also be easier.
            Hide
            rsandell rsandell added a comment -

            We could make a best effort conversion of the ssh commands. E.g. if the commands are equal to what the defaults were in the previous version then just replace them with the new command template otherwise add an admin monitor warning calling the administrator to action in changing the templates.

            Show
            rsandell rsandell added a comment - We could make a best effort conversion of the ssh commands. E.g. if the commands are equal to what the defaults were in the previous version then just replace them with the new command template otherwise add an admin monitor warning calling the administrator to action in changing the templates.
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Scott Hebert
            Path:
            src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/config/Config.java
            src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/config/IGerritHudsonTriggerConfig.java
            src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpander.java
            src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/job/rest/BuildCompletedRestCommandJob.java
            src/main/resources/com/sonyericsson/hudson/plugins/gerrit/trigger/GerritServer/index.jelly
            src/main/webapp/help-GerritBuildFailedCodeReview.html
            src/main/webapp/help-GerritBuildFailedVerified.html
            src/main/webapp/help-GerritBuildNotBuiltCodeReview.html
            src/main/webapp/help-GerritBuildNotBuiltVerified.html
            src/main/webapp/help-GerritBuildStartedCodeReview.html
            src/main/webapp/help-GerritBuildStartedVerified.html
            src/main/webapp/help-GerritBuildSuccessfulCodeReview.html
            src/main/webapp/help-GerritBuildSuccessfulVerified.html
            src/main/webapp/help-GerritBuildUnstableCodeReview.html
            src/main/webapp/help-GerritBuildUnstableVerified.html
            src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/config/ConfigTest.java
            src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderParameterizedTest.java
            src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderSkipVoteParameterTest.java
            src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderTest.java
            src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/mock/MockGerritHudsonTriggerConfig.java
            src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/mock/Setup.java
            src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/utils/MockPluginCheckerConfig.java
            http://jenkins-ci.org/commit/gerrit-trigger-plugin/3a5273ee681ad93be9f77b038b77d6523a3be9fb
            Log:
            Allow override of code-review/verified value from job

            Currently, it is NOT possible to have one Jenkins job setting only the Verified label to +1 or -1
            and another Jenkins job setting only code-review label to +1 or -1 for the same patch-set in Gerrit.

            This change will now allow:

            • Setting a code-review or verified value to empty for the Server default value
            • Setting a job specific value for a code-review or verified value
            • Only the jobs that have overridden the value in their configuration will contribute to the code-review or verified value.

            JENKINS-30367 JENKINS-30393

            Change-Id: I08d5b77fc55b49ae2d07a4eb97bbff80aa87b46b

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Scott Hebert Path: src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/config/Config.java src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/config/IGerritHudsonTriggerConfig.java src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpander.java src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/job/rest/BuildCompletedRestCommandJob.java src/main/resources/com/sonyericsson/hudson/plugins/gerrit/trigger/GerritServer/index.jelly src/main/webapp/help-GerritBuildFailedCodeReview.html src/main/webapp/help-GerritBuildFailedVerified.html src/main/webapp/help-GerritBuildNotBuiltCodeReview.html src/main/webapp/help-GerritBuildNotBuiltVerified.html src/main/webapp/help-GerritBuildStartedCodeReview.html src/main/webapp/help-GerritBuildStartedVerified.html src/main/webapp/help-GerritBuildSuccessfulCodeReview.html src/main/webapp/help-GerritBuildSuccessfulVerified.html src/main/webapp/help-GerritBuildUnstableCodeReview.html src/main/webapp/help-GerritBuildUnstableVerified.html src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/config/ConfigTest.java src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderParameterizedTest.java src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderSkipVoteParameterTest.java src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderTest.java src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/mock/MockGerritHudsonTriggerConfig.java src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/mock/Setup.java src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/utils/MockPluginCheckerConfig.java http://jenkins-ci.org/commit/gerrit-trigger-plugin/3a5273ee681ad93be9f77b038b77d6523a3be9fb Log: Allow override of code-review/verified value from job Currently, it is NOT possible to have one Jenkins job setting only the Verified label to +1 or -1 and another Jenkins job setting only code-review label to +1 or -1 for the same patch-set in Gerrit. This change will now allow: Setting a code-review or verified value to empty for the Server default value Setting a job specific value for a code-review or verified value Only the jobs that have overridden the value in their configuration will contribute to the code-review or verified value. JENKINS-30367 JENKINS-30393 Change-Id: I08d5b77fc55b49ae2d07a4eb97bbff80aa87b46b
            Hide
            scoheb Scott Hebert added a comment - - edited

            Will be present in 2.17.0

            Show
            scoheb Scott Hebert added a comment - - edited Will be present in 2.17.0

              People

              • Assignee:
                scoheb Scott Hebert
                Reporter:
                rschulz Roland Schulz
              • Votes:
                1 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: