Skip to content
Snippets Groups Projects
Commit b3b09d76 authored by Martin Lowe's avatar Martin Lowe :flag_ca:
Browse files

Update validation to make all requests validated unless ignored

This removes the ability to change strictness of validation, as well as
fixing some tests that either weren't previously firing or weren't
properly testing the behaviour. Additionally, this fixes the ignored
repos check which previously wouldn't fire properly for projects in some
cases and would result in some strange behavior.
parent 4848f7d4
No related branches found
No related tags found
1 merge request!213update: switch ECA GL check to check all commits instead of just project
Pipeline #60403 failed
Showing
with 177 additions and 116 deletions
......@@ -18,7 +18,7 @@
<quarkus.platform.version>3.8.6</quarkus.platform.version>
<surefire-plugin.version>3.1.2</surefire-plugin.version>
<maven.compiler.parameters>true</maven.compiler.parameters>
<eclipse-api-version>1.1.6</eclipse-api-version>
<eclipse-api-version>1.1.8-SNAPSHOT</eclipse-api-version>
<auto-value.version>1.10.4</auto-value.version>
<org.mapstruct.version>1.5.5.Final</org.mapstruct.version>
<sonar.sources>src/main</sonar.sources>
......
......@@ -62,7 +62,8 @@ public class CommitHelper {
}
public static MultivaluedMap<String, String> getCommitParams(ValidationRequest req, String projectId) {
return getCommitParams(req.getCommits().stream().map(Commit::getHash).toList(), projectId, req.getRepoUrl().toString());
return getCommitParams(req.getCommits().stream().map(Commit::getHash).toList(), projectId,
req.getRepoUrl() != null ? req.getRepoUrl().toString() : "");
}
public static MultivaluedMap<String, String> getCommitParams(List<String> commitShas, String projectId, String repoUrl) {
......@@ -84,7 +85,9 @@ public class CommitHelper {
*/
public static String generateRequestHash(ValidationRequest req) {
StringBuilder sb = new StringBuilder();
sb.append(req.getRepoUrl());
if (req.getRepoUrl() != null) {
sb.append(req.getRepoUrl());
}
req.getCommits().forEach(c -> sb.append(c.getHash()));
try {
return Hex.toHexString(MessageDigest.getInstance("MD5").digest(sb.toString().getBytes()));
......
......@@ -190,7 +190,6 @@ public class GithubValidationHelper {
.builder()
.setProvider(ProviderType.GITHUB)
.setRepoUrl(URI.create(repositoryUrl))
.setStrictMode(true)
.setCommits(commits
.stream()
.map(c -> Commit
......
......@@ -42,17 +42,28 @@ import jakarta.ws.rs.core.MultivaluedHashMap;
*/
@ApplicationScoped
public final class ProjectHelper {
/**
*
*/
private static final String NULL_REPO_MESSAGE = "Can not match null repo URL to projects";
private static final Logger LOGGER = LoggerFactory.getLogger(ProjectHelper.class);
@Inject
CachingService cache;
@Inject
ProjectService projects;
public List<Project> retrieveProjectsForRequest(ValidationRequest req) {
if (req.getRepoUrl() == null || StringUtils.isBlank(req.getRepoUrl().toString())) {
LOGGER.warn(NULL_REPO_MESSAGE);
return Collections.emptyList();
}
String repoUrl = req.getRepoUrl().getPath();
if (repoUrl == null) {
LOGGER.warn("Can not match null repo URL to projects");
LOGGER.warn(NULL_REPO_MESSAGE);
return Collections.emptyList();
}
return retrieveProjectsForRepoURL(repoUrl, req.getProvider());
......@@ -60,7 +71,7 @@ public final class ProjectHelper {
public List<Project> retrieveProjectsForRepoURL(String repoUrl, ProviderType provider) {
if (repoUrl == null) {
LOGGER.warn("Can not match null repo URL to projects");
LOGGER.warn(NULL_REPO_MESSAGE);
return Collections.emptyList();
}
// check for all projects that make use of the given repo
......@@ -179,8 +190,7 @@ public final class ProjectHelper {
*/
private boolean doesProjectMatchGitlabRepos(Project p, String repoUrl, String projectNamespace) {
return p.getGitlabRepos().stream().anyMatch(re -> re.getUrl() != null && re.getUrl().endsWith(repoUrl))
|| (projectNamespace.startsWith(p.getGitlab().getProjectGroup() + "/")
&& p.getGitlab().getIgnoredSubGroups().stream().noneMatch(sg -> projectNamespace.startsWith(sg + "/")));
|| projectNamespace.startsWith(p.getGitlab().getProjectGroup() + "/");
}
/**
......@@ -194,7 +204,6 @@ public final class ProjectHelper {
*/
private boolean doesProjectMatchGithubRepos(Project p, String repoUrl, String projectNamespace) {
return p.getGithubRepos().stream().anyMatch(re -> re.getUrl() != null && re.getUrl().endsWith(repoUrl))
|| (StringUtils.isNotBlank(p.getGithub().getOrg()) && projectNamespace.startsWith(p.getGithub().getOrg())
&& p.getGithub().getIgnoredRepos().stream().noneMatch(repoUrl::endsWith));
|| (StringUtils.isNotBlank(p.getGithub().getOrg()) && projectNamespace.startsWith(p.getGithub().getOrg() + "/"));
}
}
......@@ -17,7 +17,9 @@ import java.util.List;
import jakarta.annotation.Nullable;
import com.fasterxml.jackson.databind.PropertyNamingStrategies.LowerCamelCaseStrategy;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonNaming;
import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
import com.google.auto.value.AutoValue;
......@@ -28,6 +30,7 @@ import com.google.auto.value.AutoValue;
*
*/
@AutoValue
@JsonNaming(LowerCamelCaseStrategy.class)
@JsonDeserialize(builder = AutoValue_Commit.Builder.class)
public abstract class Commit {
public abstract String getHash();
......
......@@ -11,12 +11,12 @@
**********************************************************************/
package org.eclipsefoundation.git.eca.model;
import jakarta.annotation.Nullable;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
import com.google.auto.value.AutoValue;
import jakarta.annotation.Nullable;
/**
* Basic object representing a Git users data required for verification.
*
......
......@@ -36,6 +36,7 @@ import com.google.auto.value.AutoValue;
@JsonDeserialize(builder = AutoValue_ValidationRequest.Builder.class)
public abstract class ValidationRequest {
@JsonProperty("repoUrl")
@Nullable
public abstract URI getRepoUrl();
public abstract List<Commit> getCommits();
......@@ -46,8 +47,10 @@ public abstract class ValidationRequest {
@Nullable
public abstract Integer getEstimatedLoc();
@Nullable
@JsonProperty("strictMode")
@Deprecated
public abstract Boolean getStrictMode();
public abstract Builder toBuilder();
......@@ -60,7 +63,7 @@ public abstract class ValidationRequest {
@JsonPOJOBuilder(withPrefix = "set")
public abstract static class Builder {
@JsonProperty("repoUrl")
public abstract Builder setRepoUrl(URI repoUrl);
public abstract Builder setRepoUrl(@Nullable URI repoUrl);
public abstract Builder setCommits(List<Commit> commits);
......@@ -69,6 +72,7 @@ public abstract class ValidationRequest {
public abstract Builder setEstimatedLoc(@Nullable Integer estimatedLoc);
@JsonProperty("strictMode")
@Deprecated
public abstract Builder setStrictMode(@Nullable Boolean strictMode);
public abstract ValidationRequest build();
......
......@@ -45,6 +45,7 @@ public abstract class ValidationResponse {
public abstract boolean getTrackedProject();
@Deprecated
public abstract boolean getStrictMode();
@Nullable
......@@ -103,6 +104,7 @@ public abstract class ValidationResponse {
public abstract Builder setTrackedProject(boolean trackedProject);
@Deprecated
public abstract Builder setStrictMode(boolean strictMode);
public abstract Builder setFingerprint(@Nullable String fingerprint);
......
......@@ -16,7 +16,7 @@ import org.eclipsefoundation.git.eca.namespace.APIStatusCode;
*
* @author Martin Lowe
*/
public record ValidationResponsePart(CommitStatus commit, String hash, boolean isTrackedProject, boolean isStrict) {
public record ValidationResponsePart(CommitStatus commit, String hash, boolean isTrackedProject) {
/**
* Allows for the checks for tracked project + repsonse strictness to apply to messages more easily without changing the value returned
* to the user.
......@@ -25,10 +25,6 @@ public record ValidationResponsePart(CommitStatus commit, String hash, boolean i
* @param code the status code associated with the message.
*/
public void addError(String error, APIStatusCode code) {
if (this.isTrackedProject || this.isStrict) {
commit.addError(error, code);
} else {
commit.addWarning(error, code);
}
commit.addError(error, code);
}
}
......@@ -167,10 +167,6 @@ public class ValidationResource extends CommonResource {
if (req.getCommits() == null || req.getCommits().isEmpty()) {
messages.add("A commit is required to validate");
}
// check that we have a repo set
if (req.getRepoUrl() == null || StringUtils.isBlank(req.getRepoUrl().getPath())) {
messages.add("A base repo URL needs to be set in order to validate");
}
// check that we have a type set
if (req.getProvider() == null) {
messages.add("A provider needs to be set to validate a request");
......
......@@ -13,6 +13,7 @@ package org.eclipsefoundation.git.eca.service.impl;
import java.time.Duration;
import java.time.temporal.ChronoUnit;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Objects;
......@@ -31,6 +32,7 @@ import org.eclipsefoundation.git.eca.helper.CommitHelper;
import org.eclipsefoundation.git.eca.helper.ProjectHelper;
import org.eclipsefoundation.git.eca.model.Commit;
import org.eclipsefoundation.git.eca.model.CommitStatus;
import org.eclipsefoundation.git.eca.model.CommitStatus.CommitStatusMessage;
import org.eclipsefoundation.git.eca.model.GitUser;
import org.eclipsefoundation.git.eca.model.ValidationRequest;
import org.eclipsefoundation.git.eca.model.ValidationResponse;
......@@ -95,6 +97,26 @@ public class DefaultValidationService implements ValidationService {
APIStatusCode.ERROR_DEFAULT));
return r;
}
// check if we are in an ignored repo, and if so ignore processing
if (isRequestForIgnoredAsset(filteredProjects, req)) {
ValidationResponse r = generateBaseResponse(req, filteredProjects);
req
.getCommits()
.stream()
.forEach(c -> r
.getCommits()
.put(c.getHash(),
CommitStatus
.builder()
.setMessages(Arrays
.asList(CommitStatusMessage
.builder()
.setCode(APIStatusCode.SUCCESS_SKIPPED)
.setMessage("Commit is for an ignored repo, skipping")
.build()))
.build()));
return r;
}
// get previous validation status messages
List<CommitValidationStatus> statuses = statusService
......@@ -128,11 +150,7 @@ public class DefaultValidationService implements ValidationService {
*/
private List<ValidationResponsePart> doSerialProcessing(ValidationRequest req, List<Project> filteredProjects,
List<CommitValidationStatus> statuses) {
return req
.getCommits()
.stream()
.map(c -> checkAndProcessCommit(c, filteredProjects, Boolean.TRUE.equals(req.getStrictMode()), statuses))
.toList();
return req.getCommits().stream().map(c -> checkAndProcessCommit(c, filteredProjects, statuses)).toList();
}
/**
......@@ -154,9 +172,7 @@ public class DefaultValidationService implements ValidationService {
.stream()
.map(i -> Uni
.createFrom()
.completionStage(() -> executor
.supplyAsync(() -> checkAndProcessCommit(i, filteredProjects,
Boolean.TRUE.equals(req.getStrictMode()), statuses))))
.completionStage(() -> executor.supplyAsync(() -> checkAndProcessCommit(i, filteredProjects, statuses))))
.toList())
.usingConcurrencyOf(parallelProcessingConfig.threadsPerValidation())
.with(ValidationResponsePart.class, list -> list);
......@@ -170,12 +186,10 @@ public class DefaultValidationService implements ValidationService {
*
* @param c the commit to process
* @param filteredProjects tracked projects for the current request
* @param useStrict whether or not strict validation should be used when processing errors.
* @param statuses list containing historic status history for the given request
* @return the validation status part for the current commit
*/
private ValidationResponsePart checkAndProcessCommit(Commit c, List<Project> filteredProjects, boolean useStrict,
List<CommitValidationStatus> statuses) {
private ValidationResponsePart checkAndProcessCommit(Commit c, List<Project> filteredProjects, List<CommitValidationStatus> statuses) {
LOGGER.trace("Processing {}", c.getHash());
// get the current status if present
Optional<CommitValidationStatus> status = statuses.stream().filter(s -> s.getCommitHash().equals(c.getHash())).findFirst();
......@@ -183,10 +197,10 @@ public class DefaultValidationService implements ValidationService {
if (isValidationStatusCurrentAndValid(status, c)) {
CommitStatus commitStatus = CommitStatus.builder().build();
commitStatus.addMessage("Commit was previously validated, skipping processing", APIStatusCode.SUCCESS_SKIPPED);
return new ValidationResponsePart(commitStatus, c.getHash(), !filteredProjects.isEmpty(), useStrict);
return new ValidationResponsePart(commitStatus, c.getHash(), !filteredProjects.isEmpty());
}
// process the current commit
return processUnvalidatedCommit(c, filteredProjects, useStrict);
return processUnvalidatedCommit(c, filteredProjects);
}
/**
......@@ -195,12 +209,11 @@ public class DefaultValidationService implements ValidationService {
*
* @param c the commit to process
* @param filteredProjects tracked projects for the current request
* @param useStrict whether or not strict validation should be used when processing errors.
*/
private ValidationResponsePart processUnvalidatedCommit(Commit c, List<Project> filteredProjects, boolean useStrict) {
private ValidationResponsePart processUnvalidatedCommit(Commit c, List<Project> filteredProjects) {
// build the containers for the response messages
CommitStatus status = CommitStatus.builder().build();
ValidationResponsePart part = new ValidationResponsePart(status, c.getHash(), !filteredProjects.isEmpty(), useStrict);
ValidationResponsePart part = new ValidationResponsePart(status, c.getHash(), !filteredProjects.isEmpty());
// retrieve the author + committer for the current request
GitUser author = c.getAuthor();
......@@ -376,6 +389,7 @@ public class DefaultValidationService implements ValidationService {
* @return the Eclipse Account user information if found, or null if there was an error or no user exists.
*/
private EfUser getIdentifiedUser(GitUser user) {
LOGGER.error("{}", user);
// check if the external ID is set, and if so, attempt to look the user up.
if (StringUtils.isNotBlank(user.getExternalId())) {
// right now this is only supported for Github account lookups, so that will be used
......@@ -385,7 +399,7 @@ public class DefaultValidationService implements ValidationService {
return actualUser;
} else {
LOGGER
.debug("An external ID of {} was passed, but no matching Eclipse users found. Falling back on default email lookup.",
.error("An external ID of {} was passed, but no matching Eclipse users found. Falling back on default email lookup.",
user.getExternalId());
}
}
......@@ -439,6 +453,47 @@ public class DefaultValidationService implements ValidationService {
&& (c.getLastModificationDate() == null || status.get().getLastModified().equals(c.getLastModificationDate()));
}
/**
* Checks whether the request is for an ignored asset by comparing the requests repo URL to the ignored repo and subgroups fields for
* GitHub and GitLab project information.
*
* @param filteredProjects the projects associated with the current request
* @param req the current validation request
* @return true if the request falls under a projects ignored repositories, false otherwise
*/
private boolean isRequestForIgnoredAsset(List<Project> filteredProjects, ValidationRequest req) {
boolean isIgnoredRequest = false;
if (!filteredProjects.isEmpty() && StringUtils.isNotBlank(req.getRepoUrl().toString())) {
// get first project as there should be only one match
Project p = filteredProjects.get(0);
String projectNamespace = req.getRepoUrl().getPath().substring(1).toLowerCase();
// check the project for ignored repos based on the request type
switch (req.getProvider()) {
case GITHUB:
if (LOGGER.isTraceEnabled()) {
LOGGER
.trace("Checking req for repo at '{}' for ignored repos: {}", req.getRepoUrl(),
p.getGithub().getIgnoredRepos());
}
isIgnoredRequest = p.getGithub().getIgnoredRepos().stream().anyMatch(projectNamespace::equalsIgnoreCase);
break;
case GITLAB:
if (LOGGER.isTraceEnabled()) {
LOGGER
.trace("Checking req for repo at '{}' for ignored subgroups: {}", projectNamespace,
p.getGitlab().getIgnoredSubGroups());
}
isIgnoredRequest = p.getGitlab().getIgnoredSubGroups().stream().anyMatch(sg -> projectNamespace.startsWith(sg + "/"));
break;
default:
// gerrit cannot have an ignored repo as its an exact matching system
isIgnoredRequest = false;
break;
}
}
return isIgnoredRequest;
}
/**
* Generates the base validation response to be returned based on the request and the projects associated with the current request.
*
......
......@@ -101,7 +101,7 @@ public class MockProjectsAPI implements ProjectsAPI {
.setProjectLeads(Collections.emptyList())
.setGitlab(GitlabProject
.builder()
.setIgnoredSubGroups(Collections.emptyList())
.setIgnoredSubGroups(Arrays.asList("eclipse/dash-second/mirror"))
.setProjectGroup("eclipse/dash-second")
.build())
.setShortProjectId("sample.proto")
......@@ -111,7 +111,7 @@ public class MockProjectsAPI implements ProjectsAPI {
.setWebsiteRepo(Collections.emptyList())
.setLogo("logoUrl.com")
.setTags(Collections.emptyList())
.setGithub(GithubProject.builder().setOrg("").setIgnoredRepos(Collections.emptyList()).build())
.setGithub(GithubProject.builder().setOrg("dash-second").setIgnoredRepos(Arrays.asList("dash-second/mirror")).build())
.setContributors(Collections.emptyList())
.setIndustryCollaborations(Collections.emptyList())
.setReleases(Collections.emptyList())
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment