Skip to content

Improve PR information building #8908

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

Merged
merged 9 commits into from
Jun 3, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@ public class CiVisibilityRepoServices {
ciProvider = ciProviderInfo.getProvider();

CIInfo ciInfo = ciProviderInfo.buildCIInfo();
PullRequestInfo pullRequestInfo = buildPullRequestInfo(services.environment, ciProviderInfo);
PullRequestInfo pullRequestInfo =
buildPullRequestInfo(services.config, services.environment, ciProviderInfo);

if (pullRequestInfo.isNotEmpty()) {
LOGGER.info("PR detected: {}", pullRequestInfo);
Expand Down Expand Up @@ -110,18 +111,38 @@ public class CiVisibilityRepoServices {

@Nonnull
private static PullRequestInfo buildPullRequestInfo(
CiEnvironment environment, CIProviderInfo ciProviderInfo) {
PullRequestInfo ciProviderPrInfo = ciProviderInfo.buildPullRequestInfo();
if (ciProviderPrInfo.isNotEmpty()) {
return ciProviderPrInfo;
Config config, CiEnvironment environment, CIProviderInfo ciProviderInfo) {
PullRequestInfo info = buildUserPullRequestInfo(config, environment);

if (info.isComplete()) {
return info;
}

// complete with CI vars if user didn't provide all information
return PullRequestInfo.merge(info, ciProviderInfo.buildPullRequestInfo());
}

@Nonnull
private static PullRequestInfo buildUserPullRequestInfo(
Config config, CiEnvironment environment) {
PullRequestInfo userInfo =
new PullRequestInfo(
config.getGitPullRequestBaseBranch(),
config.getGitPullRequestBaseBranchSha(),
config.getGitCommitHeadSha());

if (userInfo.isComplete()) {
return userInfo;
}

// could not get PR info from CI provider,
// check if it was set manually
return new PullRequestInfo(
null,
environment.get(Constants.DDCI_PULL_REQUEST_TARGET_SHA),
environment.get(Constants.DDCI_PULL_REQUEST_SOURCE_SHA));
// logs-backend specific vars
PullRequestInfo ddCiInfo =
new PullRequestInfo(
null,
environment.get(Constants.DDCI_PULL_REQUEST_TARGET_SHA),
environment.get(Constants.DDCI_PULL_REQUEST_SOURCE_SHA));

return PullRequestInfo.merge(userInfo, ddCiInfo);
}

private static String getRepoRoot(CIInfo ciInfo, GitClient.Factory gitClientFactory) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import datadog.trace.api.git.GitInfo;
import datadog.trace.api.git.PersonInfo;
import datadog.trace.civisibility.ci.env.CiEnvironment;
import datadog.trace.util.Strings;
import javax.annotation.Nonnull;

class AppVeyorInfo implements CIProviderInfo {
Expand All @@ -25,6 +26,8 @@ class AppVeyorInfo implements CIProviderInfo {
public static final String APPVEYOR_REPO_BRANCH = "APPVEYOR_REPO_BRANCH";
public static final String APPVEYOR_PULL_REQUEST_HEAD_REPO_BRANCH =
"APPVEYOR_PULL_REQUEST_HEAD_REPO_BRANCH";
public static final String APPVEYOR_PULL_REQUEST_HEAD_COMMIT =
"APPVEYOR_PULL_REQUEST_HEAD_COMMIT";
public static final String APPVEYOR_REPO_TAG_NAME = "APPVEYOR_REPO_TAG_NAME";
public static final String APPVEYOR_REPO_COMMIT_MESSAGE_SUBJECT = "APPVEYOR_REPO_COMMIT_MESSAGE";
public static final String APPVEYOR_REPO_COMMIT_MESSAGE_BODY =
Expand Down Expand Up @@ -83,7 +86,15 @@ public CIInfo buildCIInfo() {
@Nonnull
@Override
public PullRequestInfo buildPullRequestInfo() {
return PullRequestInfo.EMPTY;
// check if PR is detected
if (Strings.isNotBlank(environment.get(APPVEYOR_PULL_REQUEST_HEAD_REPO_BRANCH))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're checking one env var, then use another, is the logic correct here?
In the buildGitBranch method we first use the head repo branch, then fall back to repo branch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is intentional, yes. The issue here is that on PRs, AppVeyor stores the base branch in APPVEYOR_REPO_BRANCH. But on non-PRs, it stores the regular branch. So we can't use it to detect if there is a PR or not. On the other hand, APPVEYOR_PULL_REQUEST_HEAD_REPO_BRANCH is only set on PRs but it's value is the source branch, not the base branch. So it uses it to check if there is actually a PR and then uses the correct value for the base branch. Will add a comment to clarify for future reference.

return new PullRequestInfo(
normalizeBranch(environment.get(APPVEYOR_REPO_BRANCH)),
null,
environment.get(APPVEYOR_PULL_REQUEST_HEAD_COMMIT));
} else {
return PullRequestInfo.EMPTY;
}
}

private String buildGitBranch(final String repoProvider) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ class BitBucketInfo implements CIProviderInfo {
public static final String BITBUCKET_GIT_COMMIT = "BITBUCKET_COMMIT";
public static final String BITBUCKET_GIT_BRANCH = "BITBUCKET_BRANCH";
public static final String BITBUCKET_GIT_TAG = "BITBUCKET_TAG";
public static final String BITBUCKET_PR_DESTINATION_BRANCH = "BITBUCKET_PR_DESTINATION_BRANCH";

private final CiEnvironment environment;

Expand Down Expand Up @@ -74,7 +75,8 @@ public CIInfo buildCIInfo() {
@Nonnull
@Override
public PullRequestInfo buildPullRequestInfo() {
return PullRequestInfo.EMPTY;
return new PullRequestInfo(
normalizeBranch(environment.get(BITBUCKET_PR_DESTINATION_BRANCH)), null, null);
}

private String buildPipelineUrl(final String repo, final String number) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

class BitriseInfo implements CIProviderInfo {

// https://devcenter.bitrise.io/en/references/available-environment-variables.html
public static final String BITRISE = "BITRISE_BUILD_SLUG";
public static final String BITRISE_PROVIDER_NAME = "bitrise";
public static final String BITRISE_PIPELINE_ID = "BITRISE_BUILD_SLUG";
Expand All @@ -31,6 +32,7 @@ class BitriseInfo implements CIProviderInfo {
public static final String BITRISE_GIT_AUTHOR_EMAIL = "GIT_CLONE_COMMIT_AUTHOR_EMAIL";
public static final String BITRISE_GIT_COMMITER_NAME = "GIT_CLONE_COMMIT_COMMITER_NAME";
public static final String BITRISE_GIT_COMMITER_EMAIL = "GIT_CLONE_COMMIT_COMMITER_EMAIL";
public static final String BITRISE_GIT_BRANCH_DEST = "BITRISEIO_GIT_BRANCH_DEST";

private final CiEnvironment environment;

Expand Down Expand Up @@ -70,7 +72,8 @@ public CIInfo buildCIInfo() {
@Nonnull
@Override
public PullRequestInfo buildPullRequestInfo() {
return PullRequestInfo.EMPTY;
return new PullRequestInfo(
normalizeBranch(environment.get(BITRISE_GIT_BRANCH_DEST)), null, null);
}

private String buildGitCommit() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ class BuddyInfo implements CIProviderInfo {
public static final String BUDDY_GIT_COMMIT_MESSAGE = "BUDDY_EXECUTION_REVISION_MESSAGE";
public static final String BUDDY_GIT_COMMIT_AUTHOR = "BUDDY_EXECUTION_REVISION_COMMITTER_NAME";
public static final String BUDDY_GIT_COMMIT_EMAIL = "BUDDY_EXECUTION_REVISION_COMMITTER_EMAIL";
public static final String BUDDY_RUN_PR_BASE_BRANCH = "BUDDY_RUN_PR_BASE_BRANCH";

private final CiEnvironment environment;

Expand Down Expand Up @@ -62,7 +63,8 @@ public CIInfo buildCIInfo() {
@Nonnull
@Override
public PullRequestInfo buildPullRequestInfo() {
return PullRequestInfo.EMPTY;
return new PullRequestInfo(
normalizeBranch(environment.get(BUDDY_RUN_PR_BASE_BRANCH)), null, null);
}

private String getPipelineId(String pipelineNumber) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ class BuildkiteInfo implements CIProviderInfo {
public static final String BUILDKITE_GIT_AUTHOR_EMAIL = "BUILDKITE_BUILD_AUTHOR_EMAIL";
public static final String BUILDKITE_AGENT_ID = "BUILDKITE_AGENT_ID";
private static final String BUILDKITE_CI_NODE_LABEL_PREFIX = "BUILDKITE_AGENT_META_DATA_";
private static final String BUILDKITE_PULL_REQUEST = "BUILDKITE_PULL_REQUEST";
private static final String BUILDKITE_PULL_REQUEST_BASE_BRANCH =
"BUILDKITE_PULL_REQUEST_BASE_BRANCH";

private final CiEnvironment environment;

Expand Down Expand Up @@ -78,9 +81,18 @@ public CIInfo buildCIInfo() {
@Nonnull
@Override
public PullRequestInfo buildPullRequestInfo() {
if (isPullRequest()) {
return new PullRequestInfo(
normalizeBranch(environment.get(BUILDKITE_PULL_REQUEST_BASE_BRANCH)), null, null);
}
return PullRequestInfo.EMPTY;
}

private boolean isPullRequest() {
String pullRequest = environment.get(BUILDKITE_PULL_REQUEST);
return pullRequest != null && !"false".equals(pullRequest);
}

private String buildCiNodeLabels() {
List<String> labels = new ArrayList<>();
for (Map.Entry<String, String> e : environment.get().entrySet()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ public CIProviderInfo createCIProviderInfo(Path currentPath) {
} else if (environment.get(AwsCodePipelineInfo.AWS_CODEPIPELINE) != null
&& environment.get(AwsCodePipelineInfo.AWS_CODEPIPELINE).startsWith("codepipeline")) {
return new AwsCodePipelineInfo(environment);
} else if (environment.get(DroneInfo.DRONE) != null) {
return new DroneInfo(environment);
} else {
return new UnknownCIInfo(environment, targetFolder, currentPath);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
import javax.annotation.Nonnull;

public class CodefreshInfo implements CIProviderInfo {

// https://codefresh.io/docs/docs/pipelines/variables/#system-variables
public static final String CODEFRESH = "CF_BUILD_ID";
public static final String CODEFRESH_PROVIDER_NAME = "codefresh";
public static final String CF_STEP_NAME = "CF_STEP_NAME";
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
package datadog.trace.civisibility.ci;

import static datadog.trace.api.git.GitUtils.filterSensitiveInfo;
import static datadog.trace.api.git.GitUtils.normalizeBranch;
import static datadog.trace.api.git.GitUtils.normalizeTag;
import static datadog.trace.civisibility.utils.FileUtils.expandTilde;

import datadog.trace.api.civisibility.telemetry.tag.Provider;
import datadog.trace.api.git.CommitInfo;
import datadog.trace.api.git.GitInfo;
import datadog.trace.api.git.PersonInfo;
import datadog.trace.civisibility.ci.env.CiEnvironment;
import javax.annotation.Nonnull;

public class DroneInfo implements CIProviderInfo {

public static final String DRONE = "DRONE";
public static final String DRONE_PROVIDER_NAME = "drone";
public static final String DRONE_BUILD_NUMBER = "DRONE_BUILD_NUMBER";
public static final String DRONE_BUILD_LINK = "DRONE_BUILD_LINK";
public static final String DRONE_STEP_NAME = "DRONE_STEP_NAME";
public static final String DRONE_STAGE_NAME = "DRONE_STAGE_NAME";
public static final String DRONE_WORKSPACE = "DRONE_WORKSPACE";
public static final String DRONE_GIT_HTTP_URL = "DRONE_GIT_HTTP_URL";
public static final String DRONE_COMMIT_SHA = "DRONE_COMMIT_SHA";
public static final String DRONE_BRANCH = "DRONE_BRANCH";
public static final String DRONE_TAG = "DRONE_TAG";
public static final String DRONE_COMMIT_AUTHOR_NAME = "DRONE_COMMIT_AUTHOR_NAME";
public static final String DRONE_COMMIT_AUTHOR_EMAIL = "DRONE_COMMIT_AUTHOR_EMAIL";
public static final String DRONE_COMMIT_MESSAGE = "DRONE_COMMIT_MESSAGE";

private final CiEnvironment environment;

DroneInfo(CiEnvironment environment) {
this.environment = environment;
}

@Override
public GitInfo buildCIGitInfo() {
return new GitInfo(
filterSensitiveInfo(environment.get(DRONE_GIT_HTTP_URL)),
normalizeBranch(environment.get(DRONE_BRANCH)),
normalizeTag(environment.get(DRONE_TAG)),
new CommitInfo(
environment.get(DRONE_COMMIT_SHA),
buildGitAuthor(),
PersonInfo.NOOP,
environment.get(DRONE_COMMIT_MESSAGE)));
}

@Override
public CIInfo buildCIInfo() {
return CIInfo.builder(environment)
.ciProviderName(DRONE_PROVIDER_NAME)
.ciPipelineNumber(environment.get(DRONE_BUILD_NUMBER))
.ciPipelineUrl(environment.get(DRONE_BUILD_LINK))
.ciJobName(environment.get(DRONE_STEP_NAME))
.ciStageName(environment.get(DRONE_STAGE_NAME))
.ciWorkspace(expandTilde(environment.get(DRONE_WORKSPACE)))
.build();
}

@Nonnull
@Override
public PullRequestInfo buildPullRequestInfo() {
return PullRequestInfo.EMPTY;
}

private PersonInfo buildGitAuthor() {
return new PersonInfo(
environment.get(DRONE_COMMIT_AUTHOR_NAME), environment.get(DRONE_COMMIT_AUTHOR_EMAIL));
}

@Override
public Provider getProvider() {
return Provider.DRONE;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ class GitLabInfo implements CIProviderInfo {
public static final String GITLAB_CI_RUNNER_TAGS = "CI_RUNNER_TAGS";
public static final String GITLAB_PULL_REQUEST_BASE_BRANCH =
"CI_MERGE_REQUEST_TARGET_BRANCH_NAME";
public static final String GITLAB_PULL_REQUEST_BASE_BRANCH_SHA =
"CI_MERGE_REQUEST_TARGET_BRANCH_SHA";
public static final String GITLAB_PULL_REQUEST_COMMIT_HEAD_SHA =
"CI_MERGE_REQUEST_SOURCE_BRANCH_SHA";

Expand Down Expand Up @@ -87,8 +85,8 @@ public CIInfo buildCIInfo() {
@Override
public PullRequestInfo buildPullRequestInfo() {
return new PullRequestInfo(
environment.get(GITLAB_PULL_REQUEST_BASE_BRANCH),
environment.get(GITLAB_PULL_REQUEST_BASE_BRANCH_SHA),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this being removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are removing the variable use in the CI spec because it stores the HEAD of the base branch, not the common ancestor where the current branch diverted from it, as we were expecting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, and then knowing the base branch we'll use the script to detect the common ancestor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. The big benefit in impacted tests is knowing the base branch. Actually knowing the base branch SHA or not is not really that important as it can be computed with a simple git command once the base branch is known. And even without considering impacted tests, it would be confusing to use git.pull_request.base_branch_sha for different things: the common ancestor when the CI is Github Actions and the base branch HEAD when running on Gitlab.

normalizeBranch(environment.get(GITLAB_PULL_REQUEST_BASE_BRANCH)),
null,
environment.get(GITLAB_PULL_REQUEST_COMMIT_HEAD_SHA));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ public PullRequestInfo buildPullRequestInfo() {

} catch (Exception e) {
LOGGER.warn("Error while parsing GitHub event", e);
return PullRequestInfo.EMPTY;
return new PullRequestInfo(baseRef, null, null);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,32 @@ public boolean isNotEmpty() {
|| Strings.isNotBlank(gitCommitHeadSha);
}

public boolean isComplete() {
return Strings.isNotBlank(pullRequestBaseBranch)
&& Strings.isNotBlank(pullRequestBaseBranchSha)
&& Strings.isNotBlank(gitCommitHeadSha);
}

/**
* Merges info by completing the empty information fields with the fallback's
*
* @param info Base PR info
* @param fallback Fallback PR info
* @return Completed PR info
*/
public static PullRequestInfo merge(PullRequestInfo info, PullRequestInfo fallback) {
return new PullRequestInfo(
Strings.isNotBlank(info.pullRequestBaseBranch)
? info.pullRequestBaseBranch
: fallback.pullRequestBaseBranch,
Strings.isNotBlank(info.pullRequestBaseBranchSha)
? info.pullRequestBaseBranchSha
: fallback.pullRequestBaseBranchSha,
Strings.isNotBlank(info.gitCommitHeadSha)
? info.gitCommitHeadSha
: fallback.gitCommitHeadSha);
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ class TravisInfo implements CIProviderInfo {
public static final String TRAVIS_GIT_BRANCH = "TRAVIS_BRANCH";
public static final String TRAVIS_GIT_TAG = "TRAVIS_TAG";
public static final String TRAVIS_GIT_COMMIT_MESSAGE = "TRAVIS_COMMIT_MESSAGE";
public static final String TRAVIS_PULL_REQUEST = "TRAVIS_PULL_REQUEST";
public static final String TRAVIS_PULL_REQUEST_HEAD_SHA = "TRAVIS_PULL_REQUEST_SHA";

private final CiEnvironment environment;

Expand Down Expand Up @@ -64,9 +66,20 @@ public CIInfo buildCIInfo() {
@Nonnull
@Override
public PullRequestInfo buildPullRequestInfo() {
if (isPullRequest()) {
return new PullRequestInfo(
normalizeBranch(environment.get(TRAVIS_GIT_BRANCH)),
null,
environment.get(TRAVIS_PULL_REQUEST_HEAD_SHA));
}
return PullRequestInfo.EMPTY;
}

private boolean isPullRequest() {
String pullRequest = environment.get(TRAVIS_PULL_REQUEST);
return pullRequest != null && !"false".equals(pullRequest);
}

private String buildGitBranch() {
final String fromBranch = environment.get(TRAVIS_GIT_PR_BRANCH);
if (fromBranch != null && !fromBranch.isEmpty()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,9 @@ abstract class CITagsProviderTest extends Specification {
CIProviderInfoFactory ciProviderInfoFactory = new CIProviderInfoFactory(Config.get(), GIT_FOLDER_FOR_TESTS, new CiEnvironmentImpl(System.getenv()))
def ciProviderInfo = ciProviderInfoFactory.createCIProviderInfo(getWorkspacePath())
def ciInfo = ciProviderInfo.buildCIInfo()
def prInfo = ciProviderInfo.buildPullRequestInfo()
def ciTagsProvider = ciTagsProvider()
def tags = ciTagsProvider.getCiTags(ciInfo, PullRequestInfo.EMPTY)
def tags = ciTagsProvider.getCiTags(ciInfo, prInfo)

then:
if (isCi()) {
Expand Down
Loading