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

234: Allow more granular jcheck reviewer configuration #368

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -38,4 +38,4 @@ domain=openjdk.org
files=.*\.java$|.*\.yml$|.*\.gradle$|.*.\txt$

[checks "reviewers"]
minimum=1
reviewers=1
@@ -118,8 +118,7 @@ private static INI convert(INI old) {
config.add("message=Merge");

config.add("[checks \"reviewers\"]");
config.add("minimum=1");
config.add("role=contributor");
config.add("contributor=1");
config.add("ignore=duke");

config.add("[checks \"committer\"]");
@@ -29,6 +29,9 @@

import java.io.IOException;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.HashMap;
import java.util.stream.Collectors;
import java.util.logging.Logger;

@@ -42,20 +45,24 @@ public class ReviewersCheck extends CommitCheck {
this.utils = utils;
}

private boolean hasRole(Project project, String role, String username, int version) {
private String nextRequiredRole(String role, Map<String, Integer> count) {
switch (role) {
case "lead":
return project.isLead(username, version);
return count.get("reviewer") != 0 ?
"reviewer" : nextRequiredRole("reviewer", count);
case "reviewer":
return project.isReviewer(username, version);
return count.get("committer") != 0 ?
"committer" : nextRequiredRole("committer", count);
case "committer":
return project.isCommitter(username, version);
return count.get("author") != 0 ?
"author" : nextRequiredRole("author", count);
case "author":
return project.isAuthor(username, version);
return count.get("contributor") != 0 ?
"contributor" : nextRequiredRole("contributor", count);
case "contributor":
return census.isContributor(username);
return null;
default:
throw new IllegalStateException("Unsupported role: " + role);
throw new IllegalArgumentException("Unexpected role: " + role);
}
}

@@ -69,22 +76,19 @@ Iterator<Issue> check(Commit commit, CommitMessage message, JCheckConfiguration
var project = census.project(conf.general().project());
var version = conf.census().version();
var domain = conf.census().domain();
var role = conf.checks().reviewers().role();
var required = conf.checks().reviewers().minimum();

var numLeadRole = conf.checks().reviewers().lead();
var numReviewerRole = conf.checks().reviewers().reviewers();
var numCommitterRole = conf.checks().reviewers().committers();
var numAuthorRole = conf.checks().reviewers().authors();
var numContributorRole = conf.checks().reviewers().contributors();

var ignore = conf.checks().reviewers().ignore();
var reviewers = message.reviewers()
.stream()
.filter(r -> !ignore.contains(r))
.collect(Collectors.toList());

var actual = reviewers.stream()
.filter(reviewer -> hasRole(project, role, reviewer, version))
.count();
if (actual < required) {
log.finer("issue: too few reviewers found");
return iterator(new TooFewReviewersIssue(Math.toIntExact(actual), required, metadata));
}

var invalid = reviewers.stream()
.filter(r -> !census.isContributor(r))
.collect(Collectors.toList());
@@ -93,6 +97,55 @@ Iterator<Issue> check(Commit commit, CommitMessage message, JCheckConfiguration
return iterator(new InvalidReviewersIssue(invalid, project, metadata));
}

var requirements = Map.of(
"lead", numLeadRole,
"reviewer", numReviewerRole,
"committer", numCommitterRole,
"author", numAuthorRole,
"contributor", numContributorRole);

var roles = new HashMap<String, String>();
for (var reviewer : reviewers) {
String role = null;
if (project.isLead(reviewer, version)) {
role = "lead";
} else if (project.isReviewer(reviewer, version)) {
role = "reviewer";
} else if (project.isCommitter(reviewer, version)) {
role = "committer";
} else if (project.isAuthor(reviewer, version)) {
role = "author";
} else if (census.isContributor(reviewer)) {
role = "contributor";
} else {
throw new IllegalStateException("No role for reviewer: " + reviewer);
}

roles.put(reviewer, role);
}

var missing = new HashMap<String, Integer>(requirements);
for (var reviewer : reviewers) {
var role = roles.get(reviewer);
if (missing.get(role) == 0) {
var next = nextRequiredRole(role, missing);
if (next != null) {
missing.put(next, missing.get(next) - 1);
}
} else {
missing.put(role, missing.get(role) - 1);
}
}

for (var role : missing.keySet()) {
int required = requirements.get(role);
int n = missing.get(role);
if (n > 0) {
log.finer("issue: too few reviewers with role " + role + " found");
return iterator(new TooFewReviewersIssue(required - n, required, role, metadata));
}
}

var username = commit.author().name();
var email = commit.author().email();
if (email != null && email.endsWith("@" + domain)) {
@@ -28,24 +28,42 @@
import java.util.stream.Collectors;

public class ReviewersConfiguration {
static final ReviewersConfiguration DEFAULT = new ReviewersConfiguration(1, "reviewer", List.of("duke"));
static final ReviewersConfiguration DEFAULT = new ReviewersConfiguration(0, 1, 0, 0, 0, List.of("duke"));

private final int minimum;
private final String role;
private final int lead;
private final int reviewers;
private final int committers;
private final int authors;
private final int contributors;
private final List<String> ignore;

ReviewersConfiguration(int minimum, String role, List<String> ignore) {
this.minimum = minimum;
this.role = role;
ReviewersConfiguration(int lead, int reviewers, int committers, int authors, int contributors, List<String> ignore) {
this.lead = lead;
this.reviewers = reviewers;
this.committers = committers;
this.authors = authors;
this.contributors = contributors;
this.ignore = ignore;
}

public int minimum() {
return minimum;
public int lead() {
return lead;
}

public String role() {
return role;
public int reviewers() {
return reviewers;
}

public int committers() {
return committers;
}

public int authors() {
return authors;
}

public int contributors() {
return contributors;
}

public List<String> ignore() {
@@ -61,9 +79,36 @@ static ReviewersConfiguration parse(Section s) {
return DEFAULT;
}

var minimum = s.get("minimum", DEFAULT.minimum());
var role = s.get("role", DEFAULT.role());
var lead = s.get("lead", DEFAULT.lead());
var reviewers = s.get("reviewers", DEFAULT.reviewers());
var committers = s.get("committers", DEFAULT.committers());
var authors = s.get("authors", DEFAULT.authors());
var contributors = s.get("contributors", DEFAULT.contributors());

if (s.contains("minimum")) {
var minimum = s.get("minimum").asInt();
if (s.contains("role")) {
var role = s.get("role").asString();
if (role.equals("lead")) {
lead = minimum;
} else if (role.equals("reviewer")) {
reviewers = minimum;
} else if (role.equals("committer")) {
committers = minimum;
} else if (role.equals("author")) {
authors = minimum;
} else if (role.equals("contributor")) {
contributors = minimum;
} else {
throw new IllegalArgumentException("Unexpected role: " + role);
}
} else {
reviewers = minimum;
}
}

var ignore = s.get("ignore", DEFAULT.ignore());
return new ReviewersConfiguration(minimum, role, ignore);

return new ReviewersConfiguration(lead, reviewers, committers, authors, contributors, ignore);
}
}
@@ -25,11 +25,13 @@
public class TooFewReviewersIssue extends CommitIssue {
private final int numActual;
private final int numRequired;
private final String role;

TooFewReviewersIssue(int numActual, int numRequired, CommitIssue.Metadata metadata) {
TooFewReviewersIssue(int numActual, int numRequired, String role, CommitIssue.Metadata metadata) {
super(metadata);
this.numActual = numActual;
this.numRequired = numRequired;
this.role = role;
}

public int numRequired() {
@@ -40,6 +42,10 @@ public int numActual() {
return numActual;
}

public String role() {
return role;
}

@Override
public void accept(IssueVisitor v) {
v.visit(this);