-
Notifications
You must be signed in to change notification settings - Fork 306
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
Changes from all commits
8747ab6
4d53a5f
ce53f26
1fde205
9751bde
9f065e7
09d3c9d
6a94235
fce06c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
---|---|---|
|
@@ -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"; | ||
|
||
|
@@ -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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this being removed? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
normalizeBranch(environment.get(GITLAB_PULL_REQUEST_BASE_BRANCH)), | ||
null, | ||
environment.get(GITLAB_PULL_REQUEST_COMMIT_HEAD_SHA)); | ||
} | ||
|
||
|
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.
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 branchThere 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.
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.