From a5f5c0be34c89dbaa82a913977f5dec8feee0898 Mon Sep 17 00:00:00 2001 From: Maggie Lou Date: Wed, 8 May 2024 10:48:38 -0500 Subject: [PATCH] [ci_runner] Support only passing a commit sha and not a branch (#6519) --- enterprise/server/cmd/ci_runner/main.go | 97 ++++++--- .../integration/ci_runner/ci_runner_test.go | 196 ++++++++++++------ enterprise/server/workflow/service/service.go | 10 +- 3 files changed, 199 insertions(+), 104 deletions(-) diff --git a/enterprise/server/cmd/ci_runner/main.go b/enterprise/server/cmd/ci_runner/main.go index 9535af89bb0..a66591db5a1 100644 --- a/enterprise/server/cmd/ci_runner/main.go +++ b/enterprise/server/cmd/ci_runner/main.go @@ -813,12 +813,13 @@ func (ar *actionRunner) Run(ctx context.Context, ws *workspace) error { if *prNumber != 0 { buildMetadata.Metadata["PULL_REQUEST_NUMBER"] = fmt.Sprintf("%d", *prNumber) } - if *targetRepoURL != "" { - buildMetadata.Metadata["REPO_URL"] = *targetRepoURL - } - if *pushedRepoURL != *targetRepoURL { + baseURL := *pushedRepoURL + isFork := *targetRepoURL != "" && *pushedRepoURL != *targetRepoURL + if isFork { + baseURL = *targetRepoURL buildMetadata.Metadata["FORK_REPO_URL"] = *pushedRepoURL } + buildMetadata.Metadata["REPO_URL"] = baseURL if *visibility != "" { buildMetadata.Metadata["VISIBILITY"] = *visibility } @@ -914,10 +915,7 @@ func (ar *actionRunner) Run(ctx context.Context, ws *workspace) error { // repo since we merge the target branch before running the workflow; // we set this for the purpose of reporting statuses to GitHub. {Key: "COMMIT_SHA", Value: *commitSHA}, - // REPO_URL is used to report statuses, so always set it to the - // target repo URL (which should be the same URL on which the workflow - // is configured). - {Key: "REPO_URL", Value: *targetRepoURL}, + {Key: "REPO_URL", Value: baseURL}, }, }}, } @@ -1616,8 +1614,8 @@ func (ws *workspace) applyPatch(ctx context.Context, bsClient bspb.ByteStreamCli } func (ws *workspace) sync(ctx context.Context) error { - if *pushedBranch == "" && *targetBranch == "" { - return status.InvalidArgumentError("expected at least one of `pushed_branch` or `target_branch` to be set") + if *pushedBranch == "" && *commitSHA == "" { + return status.InvalidArgumentError("expected at least one of `pushed_branch` or `commit_sha` to be set") } if err := ws.fetchRefs(ctx); err != nil { @@ -1650,6 +1648,8 @@ func (ws *workspace) sync(ctx context.Context) error { } *commitSHA = headCommitSHA } + // Unfortunately there's no performant way to find the branch name from + // a commit sha, so if the branch name is not set, leave it empty action, err := getActionToRun() if err != nil { @@ -1659,7 +1659,7 @@ func (ws *workspace) sync(ctx context.Context) error { // If enabled by the config, merge the target branch (if different from the // pushed branch) so that the workflow can pick up any changes not yet // incorporated into the pushed branch. - if merge && *targetRepoURL != "" && (*pushedRepoURL != *targetRepoURL || *pushedBranch != *targetBranch) { + if merge && *targetRepoURL != "" && *targetBranch != "" && (*pushedRepoURL != *targetRepoURL || *pushedBranch != *targetBranch) { targetRef := fmt.Sprintf("%s/%s", gitRemoteName(*targetRepoURL), *targetBranch) if _, err := git(ctx, ws.log, "merge", "--no-edit", targetRef); err != nil && !isAlreadyUpToDate(err) { errMsg := err.Output @@ -1699,27 +1699,38 @@ func (ws *workspace) fetchRefs(ctx context.Context) error { baseRefs = append(baseRefs, *targetBranch) } // "fork" is referring to the forked repo, if the runner was triggered by a - // PR from a fork (forkBranches will be empty otherwise). - forkBranches := make([]string, 0) + // PR from a fork (forkRefs will be empty otherwise). + forkRefs := make([]string, 0) - // Add the pushed branch to the appropriate list corresponding to the remote + // Add the pushed ref to the appropriate list corresponding to the remote // to be fetched (base or fork). - isPushedBranchInFork := *targetRepoURL != "" && *pushedRepoURL != *targetRepoURL - if isPushedBranchInFork { - forkBranches = append(forkBranches, *pushedBranch) - } else if *pushedBranch != *targetBranch { - baseRefs = append(baseRefs, *pushedBranch) + inFork := isPushedRefInFork() + if inFork { + // Try to fetch a branch if possible, because fetching + // commits that aren't at the tip of a branch requires + // users to set an additional config option (uploadpack.allowAnySHA1InWant) + pushedRefToFetch := *pushedBranch + if pushedRefToFetch == "" { + pushedRefToFetch = *commitSHA + } + forkRefs = append(forkRefs, pushedRefToFetch) + } else { + if *pushedBranch == "" { + baseRefs = append(baseRefs, *commitSHA) + } else if *pushedBranch != *targetBranch { + baseRefs = append(baseRefs, *pushedBranch) + } } baseURL := *pushedRepoURL - if isPushedBranchInFork { + if inFork { baseURL = *targetRepoURL } // TODO: Fetch from remotes in parallel if err := ws.fetch(ctx, baseURL, baseRefs); err != nil { return err } - if err := ws.fetch(ctx, *pushedRepoURL, forkBranches); err != nil { + if err := ws.fetch(ctx, *pushedRepoURL, forkRefs); err != nil { return err } return nil @@ -1733,9 +1744,15 @@ func (ws *workspace) checkoutRef(ctx context.Context) error { checkoutRef = fmt.Sprintf("%s/%s", gitRemoteName(*pushedRepoURL), *pushedBranch) } - // Create the local branch if it doesn't already exist, then update it to point to the checkout ref - if _, err := git(ctx, ws.log, "checkout", "--force", "-B", checkoutLocalBranchName, checkoutRef); err != nil { - return err + if checkoutLocalBranchName != "" { + // Create the local branch if it doesn't already exist, then update it to point to the checkout ref + if _, err := git(ctx, ws.log, "checkout", "--force", "-B", checkoutLocalBranchName, checkoutRef); err != nil { + return err + } + } else { + if _, err := git(ctx, ws.log, "checkout", checkoutRef); err != nil { + return err + } } return nil } @@ -1785,8 +1802,8 @@ func (ws *workspace) init(ctx context.Context) error { return nil } -func (ws *workspace) fetch(ctx context.Context, remoteURL string, branches []string) error { - if len(branches) == 0 { +func (ws *workspace) fetch(ctx context.Context, remoteURL string, refs []string) error { + if len(refs) == 0 { return nil } authURL, err := gitutil.AuthRepoURL(remoteURL, os.Getenv(repoUserEnvVarName), os.Getenv(repoTokenEnvVarName)) @@ -1817,8 +1834,12 @@ func (ws *workspace) fetch(ctx context.Context, remoteURL string, branches []str fetchArgs = append(fetchArgs, fmt.Sprintf("--depth=%d", *gitFetchDepth)) } fetchArgs = append(fetchArgs, remoteName) - fetchArgs = append(fetchArgs, branches...) + fetchArgs = append(fetchArgs, refs...) if _, err := git(ctx, ws.log, fetchArgs...); err != nil { + if strings.Contains(err.Error(), "Server does not allow request for unadvertised object") { + log.Warning("Git does not support fetching non-HEAD commits by default. You must either set the `uploadpack.allowAnySHA1InWant`" + + " config option in the repo that is being fetched, or set pushed_branch in the request.") + } return err } return nil @@ -1853,6 +1874,10 @@ func git(ctx context.Context, out io.Writer, args ...string) (string, *gitError) return strings.TrimSpace(output), nil } +func isPushedRefInFork() bool { + return *targetRepoURL != "" && *pushedRepoURL != *targetRepoURL +} + func formatNowUTC() string { return time.Now().UTC().Format("2006-01-02 15:04:05.000 UTC") } @@ -1881,13 +1906,19 @@ func writeBazelrc(path, invocationID string) error { } defer f.Close() + isFork := isPushedRefInFork() + + baseURL := *pushedRepoURL + if isFork { + baseURL = *targetRepoURL + } lines := []string{ "build --build_metadata=ROLE=CI", "build --build_metadata=PARENT_INVOCATION_ID=" + invocationID, // Note: these pieces of metadata are set to match the WorkspaceStatus event // for the outer (workflow) invocation. "build --build_metadata=COMMIT_SHA=" + *commitSHA, - "build --build_metadata=REPO_URL=" + *targetRepoURL, + "build --build_metadata=REPO_URL=" + baseURL, "build --build_metadata=BRANCH_NAME=" + *pushedBranch, // corresponds to GIT_BRANCH status key // Don't report commit statuses for individual bazel commands, since the // overall status of all bazel commands is reflected in the status reported @@ -1908,7 +1939,7 @@ func writeBazelrc(path, invocationID string) error { lines = append(lines, "build --build_metadata=PULL_REQUEST_NUMBER="+fmt.Sprintf("%d", *prNumber)) lines = append(lines, "build --build_metadata=DISABLE_TARGET_TRACKING=true") } - if *pushedRepoURL != *targetRepoURL { + if isFork { lines = append(lines, "build --build_metadata=FORK_REPO_URL="+*pushedRepoURL) } if apiKey := os.Getenv(buildbuddyAPIKeyEnvVarName); apiKey != "" { @@ -2084,14 +2115,18 @@ func getStructuredCommandLine() *clpb.CommandLine { } func configureGlobalCredentialHelper(ctx context.Context) error { - if !strings.HasPrefix(*targetRepoURL, "https://") { + baseURL := *pushedRepoURL + if isPushedRefInFork() { + baseURL = *targetRepoURL + } + if !strings.HasPrefix(baseURL, "https://") { return nil } repoToken := os.Getenv(repoTokenEnvVarName) if repoToken == "" { return nil } - u, err := url.Parse(*targetRepoURL) + u, err := url.Parse(baseURL) if err != nil { return nil // if URL is unparseable, do nothing } diff --git a/enterprise/server/test/integration/ci_runner/ci_runner_test.go b/enterprise/server/test/integration/ci_runner/ci_runner_test.go index d76c32f57d6..39b52d37454 100644 --- a/enterprise/server/test/integration/ci_runner/ci_runner_test.go +++ b/enterprise/server/test/integration/ci_runner/ci_runner_test.go @@ -514,7 +514,7 @@ func TestCIRunner_Push_FailedSync_CanRecoverAndRunCommand(t *testing.T) { run() } -func TestCIRunner_PullRequest_MergesTargetBranchBeforeRunning(t *testing.T) { +func TestCIRunner_Fork_MergesTargetBranchBeforeRunning(t *testing.T) { wsPath := testfs.MakeTempDir(t) targetRepoPath, _ := makeGitRepo(t, workspaceContentsWithTestsAndBuildBuddyYAML) @@ -535,13 +535,36 @@ func TestCIRunner_PullRequest_MergesTargetBranchBeforeRunning(t *testing.T) { `) commitSHA := strings.TrimSpace(testshell.Run(t, pushedRepoPath, `git rev-parse HEAD`)) - runnerFlags := []string{ + testCases := []struct { + name string + repoFlags []string + }{ + { + name: "Forked repo has branch and commit sha set", + repoFlags: []string{ + "--pushed_branch=feature", + "--commit_sha=" + commitSHA, + }, + }, + { + name: "Forked repo just has branch set", + repoFlags: []string{ + "--pushed_branch=feature", + }, + }, + { + name: "Forked repo just has commit sha set", + repoFlags: []string{ + "--commit_sha=" + commitSHA, + }, + }, + } + + baselineRunnerFlags := []string{ "--workflow_id=test-workflow", "--action_name=Test", "--trigger_event=pull_request", "--pushed_repo_url=file://" + pushedRepoPath, - "--pushed_branch=feature", - "--commit_sha=" + commitSHA, "--target_repo_url=file://" + targetRepoPath, "--target_branch=master", // Disable clean checkout fallback for this test since we expect to sync @@ -550,28 +573,20 @@ func TestCIRunner_PullRequest_MergesTargetBranchBeforeRunning(t *testing.T) { } // Start the app so the runner can use it as the BES backend. app := buildbuddy.Run(t) - runnerFlags = append(runnerFlags, app.BESBazelFlags()...) - - result := invokeRunner(t, runnerFlags, []string{}, wsPath) - - require.NotEqual(t, 0, result.ExitCode, "test should fail, so CI runner exit code should be non-zero") - - // Invoke the runner a second time in the same workspace. - result = invokeRunner(t, runnerFlags, []string{}, wsPath) - - require.NotEqual(t, 0, result.ExitCode, "test should fail, so CI runner exit code should be non-zero") + baselineRunnerFlags = append(baselineRunnerFlags, app.BESBazelFlags()...) - runnerInvocation := singleInvocation(t, app, result) - // We should be able to see both of the changes we made, since they should - // be merged together. - assert.Contains(t, runnerInvocation.ConsoleBuffer, "NONCONFLICTING_EDIT_1") - assert.Contains(t, runnerInvocation.ConsoleBuffer, "NONCONFLICTING_EDIT_2") - if t.Failed() { - t.Log(runnerInvocation.ConsoleBuffer) + for _, tc := range testCases { + runnerFlags := append(baselineRunnerFlags, tc.repoFlags...) + result := invokeRunner(t, runnerFlags, []string{}, wsPath) + runnerInvocation := singleInvocation(t, app, result) + // We should be able to see both of the changes we made, since they should + // be merged together. + assert.Contains(t, runnerInvocation.ConsoleBuffer, "NONCONFLICTING_EDIT_1", tc.name) + assert.Contains(t, runnerInvocation.ConsoleBuffer, "NONCONFLICTING_EDIT_2", tc.name) } } -func TestCIRunner_PullRequest_MergeConflict_FailsWithMergeConflictMessage(t *testing.T) { +func TestCIRunner_Fork_MergeConflict_FailsWithMergeConflictMessage(t *testing.T) { wsPath := testfs.MakeTempDir(t) targetRepoPath, _ := makeGitRepo(t, workspaceContentsWithTestsAndBuildBuddyYAML) @@ -703,29 +718,42 @@ func TestCIRunner_IgnoresInvalidFlags(t *testing.T) { } func TestRunAction_RespectsCommitSha(t *testing.T) { - wsPath := testfs.MakeTempDir(t) - repoPath, initialCommitSHA := makeGitRepo(t, workspaceContentsWithRunScript) - - baselineRunnerFlags := []string{ - "--workflow_id=test-workflow", - "--action_name=Print args", - "--trigger_event=push", - "--pushed_repo_url=file://" + repoPath, - "--pushed_branch=master", - "--target_repo_url=file://" + repoPath, - "--target_branch=master", + testCases := []struct { + setBranchName bool + }{ + { + setBranchName: true, + }, + { + setBranchName: false, + }, } - // Start the app so the runner can use it as the BES backend. - app := buildbuddy.Run(t) - baselineRunnerFlags = append(baselineRunnerFlags, app.BESBazelFlags()...) - runnerFlagsCommit1 := append(baselineRunnerFlags, "--commit_sha="+initialCommitSHA) - result := invokeRunner(t, runnerFlagsCommit1, []string{}, wsPath) - checkRunnerResult(t, result) - assert.Contains(t, result.Output, "args: {{ Hello world }}") + for _, tc := range testCases { + wsPath := testfs.MakeTempDir(t) + repoPath, initialCommitSHA := makeGitRepo(t, workspaceContentsWithRunScript) + + baselineRunnerFlags := []string{ + "--workflow_id=test-workflow", + "--action_name=Print args", + "--trigger_event=push", + "--pushed_repo_url=file://" + repoPath, + "--target_repo_url=file://" + repoPath, + } + // Start the app so the runner can use it as the BES backend. + app := buildbuddy.Run(t) + baselineRunnerFlags = append(baselineRunnerFlags, app.BESBazelFlags()...) + if tc.setBranchName { + baselineRunnerFlags = append(baselineRunnerFlags, "--pushed_branch=master", "--target_branch=master") + } - // Commit changes to the print statement in the workflow config - modifiedWorkflowConfig := ` + runnerFlagsCommit1 := append(baselineRunnerFlags, "--commit_sha="+initialCommitSHA) + result := invokeRunner(t, runnerFlagsCommit1, []string{}, wsPath) + checkRunnerResult(t, result) + assert.Contains(t, result.Output, "args: {{ Hello world }}") + + // Commit changes to the print statement in the workflow config + modifiedWorkflowConfig := ` actions: - name: "Print args" triggers: @@ -734,18 +762,19 @@ actions: bazel_commands: - run //:print_args -- "Switcheroo!" ` - newCommitSha := testgit.CommitFiles(t, repoPath, map[string]string{"buildbuddy.yaml": modifiedWorkflowConfig}) + newCommitSha := testgit.CommitFiles(t, repoPath, map[string]string{"buildbuddy.yaml": modifiedWorkflowConfig}) - // When invoked with the initial commit sha, should not contain the modified print statement - result = invokeRunner(t, runnerFlagsCommit1, []string{}, wsPath) - checkRunnerResult(t, result) - assert.Contains(t, result.Output, "args: {{ Hello world }}") + // When invoked with the initial commit sha, should not contain the modified print statement + result = invokeRunner(t, runnerFlagsCommit1, []string{}, wsPath) + checkRunnerResult(t, result) + assert.Contains(t, result.Output, "args: {{ Hello world }}") - // When invoked with the new commit sha, should contain the modified print statement - runnerFlagsCommit2 := append(baselineRunnerFlags, "--commit_sha="+newCommitSha) - result = invokeRunner(t, runnerFlagsCommit2, []string{}, wsPath) - checkRunnerResult(t, result) - assert.Contains(t, result.Output, "args: {{ Switcheroo! }}") + // When invoked with the new commit sha, should contain the modified print statement + runnerFlagsCommit2 := append(baselineRunnerFlags, "--commit_sha="+newCommitSha) + result = invokeRunner(t, runnerFlagsCommit2, []string{}, wsPath) + checkRunnerResult(t, result) + assert.Contains(t, result.Output, "args: {{ Switcheroo! }}") + } } func TestRunAction_PushedRepoOnly(t *testing.T) { @@ -753,17 +782,40 @@ func TestRunAction_PushedRepoOnly(t *testing.T) { repoPath, initialCommitSHA := makeGitRepo(t, workspaceContentsWithRunScript) testCases := []struct { - name string - useSha bool - repoFlags []string + name string + repoFlags []string + expectedReportingValues map[string]string }{ { - name: "With commit sha", - useSha: true, + name: "Pushed branch and commit sha", + repoFlags: []string{ + "--pushed_branch=master", + "--commit_sha=" + initialCommitSHA, + }, + expectedReportingValues: map[string]string{ + "branch": "master", + "commit": initialCommitSHA, + }, }, { - name: "Without commit sha", - useSha: false, + name: "Just pushed branch", + repoFlags: []string{ + "--pushed_branch=master", + }, + expectedReportingValues: map[string]string{ + "branch": "master", + "commit": initialCommitSHA, + }, + }, + { + name: "Just commit sha", + repoFlags: []string{ + "--commit_sha=" + initialCommitSHA, + }, + expectedReportingValues: map[string]string{ + "branch": "", + "commit": initialCommitSHA, + }, }, } baselineRunnerFlags := []string{ @@ -771,21 +823,35 @@ func TestRunAction_PushedRepoOnly(t *testing.T) { "--action_name=Print args", "--trigger_event=push", "--pushed_repo_url=file://" + repoPath, - "--pushed_branch=master", } // Start the app so the runner can use it as the BES backend. app := buildbuddy.Run(t) baselineRunnerFlags = append(baselineRunnerFlags, app.BESBazelFlags()...) for _, tc := range testCases { - runnerFlags := baselineRunnerFlags - if tc.useSha { - runnerFlags = append(runnerFlags, "--commit_sha="+initialCommitSHA) - } - + runnerFlags := append(baselineRunnerFlags, tc.repoFlags...) result := invokeRunner(t, runnerFlags, []string{}, wsPath) checkRunnerResult(t, result) assert.Contains(t, result.Output, "args: {{ Hello world }}", tc.name) + + // Check that metadata was reported correctly + runnerInvocation := singleInvocation(t, app, result) + var workspaceStatusEvent *bespb.WorkspaceStatus + for _, e := range runnerInvocation.Event { + if e.BuildEvent.GetWorkspaceStatus() != nil { + workspaceStatusEvent = e.BuildEvent.GetWorkspaceStatus() + break + } + } + require.NotNil(t, workspaceStatusEvent, tc.name) + + workspaceStatusMap := make(map[string]string, len(workspaceStatusEvent.Item)) + for _, i := range workspaceStatusEvent.Item { + workspaceStatusMap[i.GetKey()] = i.GetValue() + } + + require.Equal(t, tc.expectedReportingValues["branch"], workspaceStatusMap["GIT_BRANCH"], tc.name) + require.Equal(t, tc.expectedReportingValues["commit"], workspaceStatusMap["COMMIT_SHA"], tc.name) } } diff --git a/enterprise/server/workflow/service/service.go b/enterprise/server/workflow/service/service.go index 4114ca83d10..375cf9fe293 100644 --- a/enterprise/server/workflow/service/service.go +++ b/enterprise/server/workflow/service/service.go @@ -478,14 +478,8 @@ func (ws *workflowService) ExecuteWorkflow(ctx context.Context, req *wfpb.Execut if req.GetPushedRepoUrl() == "" { return nil, status.InvalidArgumentError("Missing pushed_repo_url") } - if req.GetPushedBranch() == "" { - return nil, status.InvalidArgumentError("Missing pushed_branch") - } - if req.GetTargetRepoUrl() == "" { - return nil, status.InvalidArgumentError("Missing target_repo_url") - } - if req.GetTargetBranch() == "" { - return nil, status.InvalidArgumentError("Missing target_branch") + if req.GetPushedBranch() == "" && req.GetCommitSha() == "" { + return nil, status.InvalidArgumentError("At least one of pushed_branch or commit_sha must be set.") } // Authenticate