-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add custom labels setup & config #1
base: master
Are you sure you want to change the base?
Conversation
shemuwel
commented
Mar 28, 2024
- Add workflow for published change build validation
- Add a reworked version of Itiviti@a953871 by updating UI (the way a user can configure the labels & voting values), refactored couple of classes & added fixed PR reviews from Add support for custom defined labels (verdict categories beside Verified & Code-Review) jenkinsci/gerrit-trigger-plugin#393
- Codestyle errors are still TODO (but not important now for our version of the plugin)
- for reuse in global context
- moved build status votes fields to VerdictCategory instead of having them as private fields in Config/GerritTrigger & mark their getters/setters deprecated - remove the entries for the removed status votes from jelly gerrit-trigger config & add them in VerdictCategory list instead (all the votes are set within VerdictCategory) - votes are now part of the VerdictCategory list (categories) so all deprecated methods are now searching through the list instead
- some methods are redundant (containing 'verified' name although a simpler form already exists) - extact default gerrit commands to static fields instead
- deprecate verified/code-review minimum vote calculation methods - create string based label methods for minimum voting calcs and aggregate all categories (verified & code-review included) into the templated command allowing them to be expanded later all together - remove redundant test (in which the scenario was testing the min. voting calculation for the Verified label by using its associated method instead of testing the scenario via getBuildCompletedCommand) - update tests to check for the newly added custom label
@@ -467,8 +469,8 @@ public void start() { | |||
categories = new LinkedList<VerdictCategory>(); | |||
} | |||
if (categories.isEmpty()) { | |||
categories.add(new VerdictCategory("Code-Review", "Code Review")); | |||
categories.add(new VerdictCategory("Verified", "Verified")); | |||
categories.add(new VerdictCategory(Constants.CODE_REVIEW_LABEL, Constants.CODE_REVIEW_LABEL)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong? One is "Code-Review", and the other is "Code-Review"... Or we want to display the same value as the actual label? 🤔
* Returns the value formatted as a placeholder. | ||
* @return the value formatted as a placeholder. | ||
*/ | ||
public String getPlaceholderValue() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not super clear where this is used, I can't find usages.
Integer buildStartedVote, | ||
Integer buildSuccessfulVote, | ||
Integer buildFailedVote, | ||
Integer buildUnstableVote, | ||
Integer buildNotBuiltVote, | ||
Integer buildAbortedVote) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think "VerdictCategory" should be combined with the Vote, but rather have a separate class (which potentially composes this class to store the verdict for which the votes are for)
if (result == Result.ABORTED) | ||
return BuildStatus.ABORTED; | ||
|
||
return BuildStatus.FAILED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like NOT_REGISTERED
should be here, no?
} | ||
} else if (cat instanceof JSONObject) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably throw here if none of the checks match? i.e., unknown type.