cmd/relui, internal/task: switch to Gerrit API in UpdateProxyTestRepo The original implementation of the UpdateProxyTestRepo action relied on git to directly push the test repository to Gerrit. This included force pushing of modified tags. Git force pushing is a potentially destructive operation, and by now its individual invocations need to be annotated with an explanatory rationale. We could do that, but it would further cement this action as needlessly different from how relui updates other repositories, one that may need more maintenance over time to keep functional. Instead, rewrite the action to create Gerrit CLs via the Gerrit API to modify the repository file content. This is more consistent with how we apply changes in other Gerrit hosts, and makes it possible to narrow down destructive operations from a general-purpose git force push to only tag deletion. Change-Id: I3df72f65806822cb5face5221e175a024fd9f7af Reviewed-on: https://go-review.googlesource.com/c/build/+/722620 Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Robert Findley <rfindley@google.com>
diff --git a/cmd/relui/main.go b/cmd/relui/main.go index 8f0fa6d..996006d 100644 --- a/cmd/relui/main.go +++ b/cmd/relui/main.go
@@ -151,6 +151,10 @@ Gitiles: "https://go-internal.googlesource.com", Client: gerrit.NewClient("https://go-internal-review.googlesource.com", gerrit.OAuth2Auth(creds.TokenSource)), } + modproxyTestGerritClient := &task.RealGerritClient{ + Gitiles: "https://golang-modproxy-test.googlesource.com", + Client: gerrit.NewClient("https://golang-modproxy-test-review.googlesource.com", gerrit.OAuth2Auth(creds.TokenSource)), + } gitClient := &task.Git{} gitClient.UseOAuth2Auth(creds.TokenSource) var mailFunc func(*workflow.TaskContext, task.MailHeader, task.MailContent) error @@ -311,9 +315,15 @@ CloudBuild: cloudBuildClient, }, UpdateProxyTestRepoTasks: task.UpdateProxyTestRepoTasks{ - Git: gitClient, - GerritURL: "https://golang-modproxy-test.googlesource.com/latest-go-version", - Branch: "main", + Gerrit: modproxyTestGerritClient, + Project: "latest-go-version", Branch: "main", + ChangeLink: func(changeID string) string { + parts := strings.SplitN(changeID, "~", 3) + if len(parts) != 2 { + return fmt.Sprintf("(unparseable change ID %q)", changeID) + } + return "https://golang-modproxy-test-review.googlesource.com/c/latest-go-version/+/" + parts[1] + }, }, } cycleTasks := task.ReleaseCycleTasks{
diff --git a/gerrit/gerrit.go b/gerrit/gerrit.go index 8710f3a..4668fe1 100644 --- a/gerrit/gerrit.go +++ b/gerrit/gerrit.go
@@ -1007,6 +1007,12 @@ return res, err } +// DeleteTag deletes a tag on project. +// See https://gerrit-review.googlesource.com/Documentation/rest-api-projects.html#delete-tag. +func (c *Client) DeleteTag(ctx context.Context, project, tag string) error { + return c.do(ctx, nil, http.MethodDelete, fmt.Sprintf("/projects/%s/tags/%s", project, url.PathEscape(tag)), wantResStatus(http.StatusNoContent)) +} + // GetAccountInfo gets the specified account's information from Gerrit. // For the API call, see https://gerrit-review.googlesource.com/Documentation/rest-api-accounts.html#get-account // The accountID is https://gerrit-review.googlesource.com/Documentation/rest-api-accounts.html#account-id
diff --git a/internal/relui/workflows.go b/internal/relui/workflows.go index bfe6ce7..590a8b7 100644 --- a/internal/relui/workflows.go +++ b/internal/relui/workflows.go
@@ -436,9 +436,7 @@ } addCommTasks(wd, build, comm, r.kind, wf.Slice(published), securitySummary, securityFixes, coordinators) if r.major >= currentMajor { - // Add a task for updating the module proxy test repo that makes sure modules containing go directives - // of the latest published version are fetchable. - wf.Task1(wd, "update-proxy-test", version.UpdateProxyTestRepo, published) + wf.Action1(wd, "update-proxy-test", version.UpdateProxyTestRepo, published) } h.RegisterDefinition(fmt.Sprintf("Go 1.%d %s", r.major, r.suffix), wd) @@ -485,7 +483,7 @@ securitySummary := wf.Param(wd, securitySummaryParameter) securityFixes := wf.Param(wd, securityFixesParameter) addCommTasks(wd, build, comm, task.KindMinor, wf.Slice(currPublished, prevPublished), securitySummary, securityFixes, coordinators) - wf.Task1(wd, "update-proxy-test", version.UpdateProxyTestRepo, currPublished) + wf.Action1(wd, "update-proxy-test", version.UpdateProxyTestRepo, currPublished) return wd, nil }
diff --git a/internal/task/fakes.go b/internal/task/fakes.go index 549a9ab..adbd87b 100644 --- a/internal/task/fakes.go +++ b/internal/task/fakes.go
@@ -377,6 +377,15 @@ return nil } +func (g *FakeGerrit) ForceTag(ctx context.Context, project, tag, commit string) error { + repo, err := g.repo(project) + if err != nil { + return err + } + repo.runGit("tag", "--force", tag, commit) + return nil +} + func (g *FakeGerrit) GetCommitsInRefs(ctx context.Context, project string, commits, refs []string) (map[string][]string, error) { repo, err := g.repo(project) if err != nil {
diff --git a/internal/task/gerrit.go b/internal/task/gerrit.go index c7181d9..7fe867f 100644 --- a/internal/task/gerrit.go +++ b/internal/task/gerrit.go
@@ -24,6 +24,7 @@ type GerritClient interface { // GitilesURL returns the URL to the Gitiles server for this Gerrit instance. GitilesURL() string + // CreateAutoSubmitChange creates a change with the given metadata and // contents, starts trybots with auto-submit enabled, and returns its change ID. // If the content of a file is empty, that file will be deleted from the repository. @@ -40,13 +41,15 @@ // Submitted checks if the specified change has been submitted or failed // trybots. If the CL is submitted, returns the submitted commit hash. // If parentCommit is non-empty, the submitted CL's parent must match it. - Submitted(ctx context.Context, changeID, parentCommit string) (string, bool, error) + Submitted(ctx context.Context, changeID, parentCommit string) (commit string, submitted bool, _ error) + // GetTag returns tag information for a specified tag. GetTag(ctx context.Context, project, tag string) (gerrit.TagInfo, error) // Tag creates a tag on project at the specified commit. Tag(ctx context.Context, project, tag, commit string) error // ListTags returns all the tags on project. ListTags(ctx context.Context, project string) ([]string, error) + // ReadBranchHead returns the head of a branch in project. // If the branch doesn't exist, it returns an error matching gerrit.ErrResourceNotExist. ReadBranchHead(ctx context.Context, project, branch string) (string, error) @@ -54,24 +57,27 @@ ListBranches(ctx context.Context, project string) ([]gerrit.BranchInfo, error) // CreateBranch create the given branch and returns the created branch's revision. CreateBranch(ctx context.Context, project, branch string, input gerrit.BranchInput) (string, error) + // ListProjects lists all the projects on the server. ListProjects(ctx context.Context) ([]string, error) + // ReadFile reads a file from project at the specified commit. // If the file doesn't exist, it returns an error matching gerrit.ErrResourceNotExist. ReadFile(ctx context.Context, project, commit, file string) ([]byte, error) // ReadDir reads a directory from project at the specified commit. // If the directory doesn't exist, it returns an error matching gerrit.ErrResourceNotExist. ReadDir(ctx context.Context, project, commit, dir string) ([]struct{ Name string }, error) - // GetCommitsInRefs gets refs in which the specified commits were merged into. - GetCommitsInRefs(ctx context.Context, project string, commits, refs []string) (map[string][]string, error) + // QueryChanges gets changes which match the query. QueryChanges(ctx context.Context, query string) ([]*gerrit.ChangeInfo, error) - // SetHashtags modifies the hashtags for a CL. - SetHashtags(ctx context.Context, changeID string, hashtags gerrit.HashtagsInput) error // GetChange gets information about a specific change. GetChange(ctx context.Context, changeID string, opts ...gerrit.QueryChangesOpt) (*gerrit.ChangeInfo, error) // SubmitChange submits a specific change. SubmitChange(ctx context.Context, changeID string) (gerrit.ChangeInfo, error) + + // SetHashtags modifies the hashtags for a CL. + SetHashtags(ctx context.Context, changeID string, hashtags gerrit.HashtagsInput) error + // CreateCherryPick creates a cherry-pick change. If there are no merge // conflicts, it starts trybots. If commitMessage is provided, the commit // message is updated, otherwise it is taken from the original change. @@ -86,6 +92,18 @@ GetRevisionActions(ctx context.Context, changeID, revision string) (map[string]*gerrit.ActionInfo, error) // GetCommitMessage retrieves the commit message for a change. GetCommitMessage(ctx context.Context, changeID string) (string, error) + // GetCommitsInRefs gets refs in which the specified commits were merged into. + GetCommitsInRefs(ctx context.Context, project string, commits, refs []string) (map[string][]string, error) +} + +// DestructiveGerritClient is a Gerrit client with destructive operations included. +// It's intended to be used in contexts where it's safe, such as test repositories. +type DestructiveGerritClient interface { + GerritClient + + // ForceTag forcibly creates a tag on project at the specified commit, + // even if the tag already exists and points to a different commit. + ForceTag(ctx context.Context, project, tag, commit string) error } type RealGerritClient struct { @@ -218,6 +236,39 @@ return err } +func (c *RealGerritClient) ForceTag(ctx context.Context, project, tag, commit string) error { + var needToDeleteExistingTag bool + + // Check for an existing tag. + info, err := c.Client.GetTag(ctx, project, tag) + if errors.Is(err, gerrit.ErrResourceNotExist) { + // OK. We'll be creating it below. + } else if err != nil { + return fmt.Errorf("error checking if tag %q already exists: %v", tag, err) + } else { + if info.Revision == commit { + // Nothing to do. + return nil + } + // The tag exists but points elsewhere, so it needs to be deleted before being recreated. + needToDeleteExistingTag = true + } + + // Delete its previous version if needed. + if needToDeleteExistingTag { + err := c.Client.DeleteTag(ctx, project, tag) + if err != nil { + return err + } + } + + // Create new tag. + _, err = c.Client.CreateTag(ctx, project, tag, gerrit.TagInput{ + Revision: commit, + }) + return err +} + func (c *RealGerritClient) ListTags(ctx context.Context, project string) ([]string, error) { tags, err := c.Client.GetProjectTags(ctx, project) if err != nil {
diff --git a/internal/task/updateproxytestrepo.go b/internal/task/updateproxytestrepo.go index 3bf2c4e..38e594c 100644 --- a/internal/task/updateproxytestrepo.go +++ b/internal/task/updateproxytestrepo.go
@@ -7,56 +7,81 @@ import ( "fmt" goversion "go/version" - "os" - "path/filepath" "strings" + "time" + "golang.org/x/build/gerrit" wf "golang.org/x/build/internal/workflow" "golang.org/x/mod/modfile" ) type UpdateProxyTestRepoTasks struct { - Git *Git - GerritURL string - Branch string + Gerrit DestructiveGerritClient // destructive needed to overwrite v1.0.0 tag + Project string + Branch string + // ChangeLink is an optional function to customize the URL of the review page + // for the CL with the given change ID (in <project>~<changeNumber> format). + ChangeLink func(changeID string) string } -func (t *UpdateProxyTestRepoTasks) UpdateProxyTestRepo(ctx *wf.TaskContext, published Published) (string, error) { - repo, err := t.Git.Clone(ctx, t.GerritURL) +// UpdateProxyTestRepo updates the module proxy test repo whose purpose is to make +// sure modules containing go directives of the latest published version are fetchable. +func (t *UpdateProxyTestRepoTasks) UpdateProxyTestRepo(ctx *wf.TaskContext, published Published) error { + // Read the go.mod file and check if version is higher. + head, err := t.Gerrit.ReadBranchHead(ctx, t.Project, t.Branch) if err != nil { - return "", err + return err } - - // Read the file and check if version is higher. - modFile := filepath.Join(repo.dir, "go.mod") - contents, err := os.ReadFile(modFile) + ctx.Printf("Using commit %q as the branch head.", head) + b, err := t.Gerrit.ReadFile(ctx, t.Project, head, "go.mod") if err != nil { - return "", err + return err } - f, err := modfile.ParseLax(modFile, contents, nil) + f, err := modfile.ParseLax("", b, nil) if err != nil { - return "", err + return err } - // If the published version is lower than the current go.mod version, don't update. - // If we could parse the go.mod file, assume we should update. + // If the published version is lower than the current go.mod's go directive version, don't update. if f.Go != nil && goversion.Compare(published.Version, "go"+f.Go.Version) < 0 { - return "no update", nil + ctx.Printf("No update needed.") + return nil } - version := strings.TrimPrefix(published.Version, "go") - // Update the go.mod file for the new release. - if err := os.WriteFile(modFile, []byte(fmt.Sprintf("module test\n\ngo %s\n", version)), 0666); err != nil { - return "", err + // What's left at this point is to mail a Gerrit CL, and, after it's submitted, update a tag. + // Out of abundance of caution, if something during those steps produces an unexpected error, + // disable automatic retries. A human can inspect what happened and retry manually as needed. + ctx.DisableRetries() + + // Create the go.mod file update CL and await its submission. + changeID, err := t.Gerrit.CreateAutoSubmitChange(ctx, gerrit.ChangeInput{ + Project: t.Project, Branch: t.Branch, + Subject: fmt.Sprintf("update go version to %s", published.Version), + }, nil, map[string]string{ + "go.mod": fmt.Sprintf("module test\n\ngo %s\n", strings.TrimPrefix(published.Version, "go")), + }) + if err != nil { + return err } - if _, err := repo.RunCommand(ctx, "commit", "-am", fmt.Sprintf("update go version to %s", version)); err != nil { - return "", fmt.Errorf("git commit error: %v", err) + ctx.Printf("Awaiting review/submit of %s.", t.changeLink(changeID)) + submitted, err := AwaitCondition(ctx, time.Minute, func() (string, bool, error) { + return t.Gerrit.Submitted(ctx, changeID, "") + }) + if err != nil { + return err } - // Force move the tag. - if _, err := repo.RunCommand(ctx, "tag", "-af", "v1.0.0", "-m", fmt.Sprintf("moving tag to include go version %s", version)); err != nil { - return "", fmt.Errorf("git tag error: %v", err) + // Forcibly update the v1.0.0 tag. + err = t.Gerrit.ForceTag(ctx, t.Project, "v1.0.0", submitted) + if err != nil { + return err } - if _, err := repo.RunCommand(ctx, "push", "--force", "--tags", "origin", t.Branch); err != nil { - return "", fmt.Errorf("git push --tags error: %v", err) + ctx.Printf("Updated the v1.0.0 tag of %q test repo to point to a commit whose go directive is %q.", t.Project, strings.TrimPrefix(published.Version, "go")) + + return nil +} + +func (t *UpdateProxyTestRepoTasks) changeLink(changeID string) string { + if t.ChangeLink != nil { + return t.ChangeLink(changeID) } - return "updated", nil + return ChangeLink(changeID) }
diff --git a/internal/task/updateproxytestrepo_test.go b/internal/task/updateproxytestrepo_test.go index 5bb44e1..42d8b3b 100644 --- a/internal/task/updateproxytestrepo_test.go +++ b/internal/task/updateproxytestrepo_test.go
@@ -29,8 +29,6 @@ t.Run(tt.name, func(t *testing.T) { fakeRepo := NewFakeRepo(t, "fake") fakeGerrit := NewFakeGerrit(t, fakeRepo) - // We need to do this so we can push to the branch we checked out. - fakeRepo.runGit("config", "receive.denyCurrentBranch", "updateInstead") fakeRepo.CommitOnBranch("master", map[string]string{ "go.mod": fmt.Sprintf("module test\n\ngo %s\n", tt.old), @@ -38,13 +36,16 @@ fakeRepo.Tag("v1.0.0", "master") upgradeGoVersion := &UpdateProxyTestRepoTasks{ - Git: &Git{}, - GerritURL: fakeRepo.dir.dir, - Branch: "master", + Gerrit: fakeGerrit, + Project: fakeRepo.name, + Branch: "master", } - ctx := context.Background() - if _, err := upgradeGoVersion.UpdateProxyTestRepo(&workflow.TaskContext{Context: ctx}, Published{Version: "go" + tt.new}); err != nil { + ctx := &workflow.TaskContext{ + Context: context.Background(), + Logger: &testLogger{t, ""}, + } + if err := upgradeGoVersion.UpdateProxyTestRepo(ctx, Published{Version: "go" + tt.new}); err != nil { t.Fatal(err) } @@ -70,7 +71,6 @@ if string(value) != want { t.Errorf("expected %q, got %q", want, string(value)) } - } tag, err := fakeGerrit.GetTag(ctx, fakeRepo.name, "v1.0.0")