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

Add custom labels setup & config #1

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add custom labels setup & config #1

wants to merge 6 commits into from

Conversation

shemuwel
Copy link
Collaborator

- 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));
Copy link

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() {
Copy link

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.

Comment on lines +64 to +69
Integer buildStartedVote,
Integer buildSuccessfulVote,
Integer buildFailedVote,
Integer buildUnstableVote,
Integer buildNotBuiltVote,
Integer buildAbortedVote) {
Copy link

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;
Copy link

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) {
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants