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

Allow override of code-review/verified value from job #255

Merged
merged 1 commit into from Oct 26, 2015

Conversation

Projects
None yet
3 participants
@scoheb
Copy link
Contributor

commented Oct 22, 2015

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]

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Oct 23, 2015

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@@ -272,6 +272,11 @@ private String expandParameters(String gerritCommand, Run r, TaskListener taskLi
for (Map.Entry<String, String> param : parameters.entrySet()) {
gerritCommand = gerritCommand.replace("<" + param.getKey() + ">", param.getValue());
}
//replace null and Integer.MAX_VALUE code review value

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 23, 2015

Member

nasty, but understandable ;)

This comment has been minimized.

Copy link
@scoheb

scoheb Oct 23, 2015

Author Contributor

I totally agree...I have been cringing since I submitted this PR ;)

@@ -365,7 +370,7 @@ protected int getVerifiedValue(Result res, GerritTrigger trigger) {
* @return the lowest verified value.
*/
public int getMinimumVerifiedValue(MemoryImprint memoryImprint, boolean onlyBuilt) {

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 23, 2015

Member

missed changing to Integer here

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 23, 2015

Member

If possible, to be more correct I think this method should return null if no minimum value is found at all. It would be more correct than returning Integer.MAX_VALUE that is definitely not a number that was found.

The method should also be annotated with CheckForNull to help findbugs, and the javadoc should be updated to note the new behaviour.

This comment has been minimized.

Copy link
@scoheb

scoheb Oct 23, 2015

Author Contributor

ok will do.

@@ -398,8 +402,8 @@ public int getMinimumVerifiedValue(MemoryImprint memoryImprint, boolean onlyBuil
* @param onlyBuilt only count builds that completed (no NOT_BUILT builds)
* @return the lowest code review value.
*/
public int getMinimumCodeReviewValue(MemoryImprint memoryImprint, boolean onlyBuilt) {
int codeReview = Integer.MAX_VALUE;
public Integer getMinimumCodeReviewValue(MemoryImprint memoryImprint, boolean onlyBuilt) {

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 23, 2015

Member

same here if possible.

gerritBuildNotBuiltCodeReviewValue = formData.optInt(
"gerritBuildNotBuiltCodeReviewValue",
DEFAULT_GERRIT_BUILD_NOT_BUILT_CODE_REVIEW_VALUE);
if (formData.isEmpty()) {

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 23, 2015

Member

Why the difference in handling verified vs code review values?

This comment has been minimized.

Copy link
@scoheb

scoheb Oct 23, 2015

Author Contributor

I guess our view was that the new default/standard for Code Review values should be that the Server values should start off as empty for a new server.

Most users associate build health with +1/-1 for Verified and not Code Review. In other words, sending Code Review values should be disabled by default.

Does that sounds reasonable?

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 23, 2015

Member

Well we could look at it another way that gerrit does not come configured with the Verified label out of the box, so having nothing set for verified and only sensible defaults for Code Review should be the proper way to go for new users :)

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 23, 2015

Member

Then we would remove the obstacle/bug of not being able to run the plugin on a brand spanking new Gerrit server :)

This comment has been minimized.

Copy link
@scoheb

scoheb Oct 23, 2015

Author Contributor

How about we compromise? We keep the defaults for both Verified and CodeReview?

This way it will work for new Gerrit server installs, but it will not break the environments (company starts with an 'E' ;) ) that expect default Verified values.

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 23, 2015

Member

I don't think we would need to compromise if the variables are treated equally.
formData.isEmpty should only happen on the first creation of a new default server. If that only creates sensible default values for Code Review I don't think it would matter to you as you only need to configure that once, any other server could then just be copied from a previous server config with the correct values for your setup.
Any existing configurations should "just work" with the previous values when you start up with the new version.

Or am I misinterpreting this section of code completely?

This comment has been minimized.

Copy link
@scoheb

scoheb Oct 23, 2015

Author Contributor

I think we are saying the same thing...

We make use of the forData.isEmpty construct for BOTH Verified and CodeReview cases.

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 26, 2015

Member

I was more thinking that the default values for all verified should be null so they would work better with a default Gerrit setup.

This comment has been minimized.

Copy link
@scoheb

scoheb Oct 26, 2015

Author Contributor

Do you think we could keep the setup for default values as it was prior to this PR?

I can create a new PR that could address correcting the default value setup to be similar to Gerrit.

This comment has been minimized.

Copy link
@rsandell
@@ -113,35 +113,35 @@
<f:textbox name="gerritBuildStartedVerifiedValue"
value="${it.config.gerritBuildStartedVerifiedValue}"
default="${com.sonyericsson.gerrithudsontrigger.config.Config.DEFAULT_GERRIT_BUILD_STARTED_VERIFIED_VALUE}"

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 23, 2015

Member

I don't think all of these default values are possible any more.

I'm going to add some inherital sin on you here and ask you if you can clean the default values up a bit, because they have probably never really worked. I doubt any of them are useful and maybe could just be removed since a new server config should be instantiated with default values from the beginning.

This comment has been minimized.

Copy link
@scoheb

scoheb Oct 23, 2015

Author Contributor

you mean the textbox default values? Yes. I will clean it up.

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 23, 2015

Member

Yes the textbox defaults. You should maybe also look over the defaults that are set when a new server config is created.

@@ -1 +1,3 @@
The default Code Review vote that should be set when a build fails.
<br><br>
If this field is left as an empty string, then any job's configured value will have precedence.

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 23, 2015

Member

Maybe should also add what the result is if no job has it configured as well.

This comment has been minimized.

Copy link
@scoheb

scoheb Oct 23, 2015

Author Contributor

will do.

@rsandell

This comment has been minimized.

Copy link
Member

commented Oct 23, 2015

Now that was code I feel I haven't seen in a long time :)

Great work!

@scoheb scoheb force-pushed the scoheb:label-clash branch 2 times, most recently from b33b952 to 63555e4 Oct 25, 2015

@scoheb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2015

@rsandell ... Can you please take a look now?

@rsandell

This comment has been minimized.

Copy link
Member

commented Oct 26, 2015

Will do

assertEquals(parameter.expectedCodeReview, result);
Integer result = instance.getMinimumCodeReviewValue(parameter.memoryImprint, true);
if (parameter.expectedCodeReview == null) {
assertEquals(null, result);

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 26, 2015

Member

assertNull(result); would be more descriptive I guess.

assertEquals(parameter.expectedVerified, result);
Integer result = instance.getMinimumVerifiedValue(parameter.memoryImprint, true);
if (parameter.expectedVerified == null) {
assertEquals(null, result);

This comment has been minimized.

Copy link
@rsandell

rsandell Oct 26, 2015

Member

assertNull(result); would be more descriptive I guess.

Scott Hebert
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

@scoheb scoheb force-pushed the scoheb:label-clash branch from 63555e4 to 3a5273e Oct 26, 2015

@scoheb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2015

I fixed the assertNulls. Please review/merge at your convenience.

@rsandell

This comment has been minimized.

Copy link
Member

commented Oct 26, 2015

OK, I'll merge as soon as Jenkins says the build is OK.

Should I wait with release until #254 and the "new defaults" are also merged?

@scoheb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2015

I just closed #254 .

I would prefer a release BEFORE the "new defaults".

@rsandell

This comment has been minimized.

Copy link
Member

commented Oct 26, 2015

ok, releasing asap ;)

rsandell added a commit that referenced this pull request Oct 26, 2015

Merge pull request #255 from scoheb/label-clash
Allow override of code-review/verified value from job

@rsandell rsandell merged commit 9e92f86 into jenkinsci:master Oct 26, 2015

1 check passed

Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.