[recipe_bootstrap] Ensure Build.Input.GitilesCommit
Previously, if we detected a recipe change, we would set the
recipe_version property to the GerritChange and checkout directly
without rebasing. This sort of works, but may lead to inconsistent
checkouts between parent and child builds if recipes.git HEAD moves.
This change reworks the strategy of checking out changes by ensuring
that the build input has a GitilesCommit with a resolved HEAD in _all_
cases, recipe change or otherwise. This essentially performs the role
of build_input_resolver/api.py.
This is a nice-to-have for non-recipe changes, as we move the resolution
further up in the pipeline. build_input_resolver will no-op when it sees
an existing GitilesCommit.
For recipe changes, this is essential, because the checkout and rebase
of recipes happens in the execution of this tool. If we wait until the
recipe has already started to do any kind of rebasing for recipes.git,
we're too late.
See go/recipe-versioning-in-recipes-cq for the relevant design.
Bug: 58899, 61798
Change-Id: I7fd719f2f91a963cc03c914de3a1ce988544136e
Reviewed-on: https://fuchsia-review.googlesource.com/c/infra/infra/+/439655
Commit-Queue: Anthony Fandrianto <atyfto@google.com>
Reviewed-by: Oliver Newman <olivernewman@google.com>
diff --git a/cmd/recipe_bootstrap/checkout/checkout.go b/cmd/recipe_bootstrap/checkout/checkout.go
index ce254da..ecacf46 100644
--- a/cmd/recipe_bootstrap/checkout/checkout.go
+++ b/cmd/recipe_bootstrap/checkout/checkout.go
@@ -14,18 +14,29 @@
buildbucketpb "go.chromium.org/luci/buildbucket/proto"
)
-// Set a build input's GitilesCommit corresponding to a remote and ref.
-func SetGitilesCommit(buildInput *buildbucketpb.Build_Input, remote string, ref string) error {
- remoteURL, err := url.Parse(remote)
- if err != nil {
- return fmt.Errorf("could not parse URL %s", remote)
+// ResolveRepo returns the host and project associated with the build's input.
+//
+// If the build input has a GitilesCommit, return its host and project.
+// If the build input only has a GerritChange, return its host and project.
+// If the build input has neither, return the host and project based on the
+// provided fallback remote.
+func ResolveRepo(buildInput *buildbucketpb.Build_Input, fallbackRemote string) (string, string, error) {
+ var host, project string
+ if buildInput.GitilesCommit != nil {
+ return buildInput.GitilesCommit.Host, buildInput.GitilesCommit.Project, nil
}
- buildInput.GitilesCommit = &buildbucketpb.GitilesCommit{
- Host: remoteURL.Host,
- Project: strings.TrimLeft(remoteURL.Path, "/"),
- Id: ref,
+ if len(buildInput.GerritChanges) > 0 {
+ host = checkout.RemoveCodeReviewSuffix(buildInput.GerritChanges[0].Host)
+ project = buildInput.GerritChanges[0].Project
+ } else {
+ remoteURL, err := url.Parse(fallbackRemote)
+ if err != nil {
+ return "", "", fmt.Errorf("could not parse remote %s as URL: %v", fallbackRemote, err)
+ }
+ host = remoteURL.Host
+ project = strings.TrimLeft(remoteURL.Path, "/")
}
- return nil
+ return host, project, nil
}
// Resolve a build input's integration URL.
@@ -45,12 +56,16 @@
}, nil
}
-// Check whether a change is a recipe change.
-func IsRecipeChange(change *buildbucketpb.GerritChange, remote string) (bool, error) {
+// Check whether a build input has a GerritChange from `remote`.
+func HasRepoChange(buildInput *buildbucketpb.Build_Input, remote string) (bool, error) {
+ if len(buildInput.GerritChanges) == 0 {
+ return false, nil
+ }
remoteURL, err := url.Parse(remote)
if err != nil {
return false, fmt.Errorf("could not parse URL %s", remote)
}
+ change := buildInput.GerritChanges[0]
if change.Project == strings.TrimLeft(remoteURL.Path, "/") && checkout.RemoveCodeReviewSuffix(change.GetHost()) == remoteURL.Host {
return true, nil
}
diff --git a/cmd/recipe_bootstrap/checkout/checkout_test.go b/cmd/recipe_bootstrap/checkout/checkout_test.go
index 3325fe6..dc2be34 100644
--- a/cmd/recipe_bootstrap/checkout/checkout_test.go
+++ b/cmd/recipe_bootstrap/checkout/checkout_test.go
@@ -6,39 +6,88 @@
import (
"net/url"
- "reflect"
"testing"
buildbucketpb "go.chromium.org/luci/buildbucket/proto"
)
-func TestSetGitilesCommit(t *testing.T) {
+func TestResolveRepo(t *testing.T) {
t.Parallel()
var tests = []struct {
- input *buildbucketpb.Build_Input
- remote string
- ref string
- expected *buildbucketpb.Build_Input
+ input *buildbucketpb.Build_Input
+ fallbackRemote string
+ expectedHost string
+ expectedProject string
+ expectErr bool
}{
{
- &buildbucketpb.Build_Input{},
- "https://fuchsia.googlesource.com/project",
- "foo",
&buildbucketpb.Build_Input{
GitilesCommit: &buildbucketpb.GitilesCommit{
Host: "fuchsia.googlesource.com",
Project: "project",
- Id: "foo",
},
},
+ "https://fuchsia.googlesource.com/integration",
+ "fuchsia.googlesource.com",
+ "project",
+ false,
+ },
+ {
+ &buildbucketpb.Build_Input{
+ GitilesCommit: &buildbucketpb.GitilesCommit{
+ Host: "fuchsia.googlesource.com",
+ Project: "project",
+ },
+ GerritChanges: []*buildbucketpb.GerritChange{
+ {
+ Host: "fuchsia-review.googlesource.com",
+ Project: "should-be-ignored",
+ },
+ },
+ },
+ "https://fuchsia.googlesource.com/integration",
+ "fuchsia.googlesource.com",
+ "project",
+ false,
+ },
+ {
+ &buildbucketpb.Build_Input{
+ GerritChanges: []*buildbucketpb.GerritChange{
+ {
+ Host: "fuchsia-review.googlesource.com",
+ Project: "project",
+ },
+ },
+ },
+ "https://fuchsia.googlesource.com/integration",
+ "fuchsia.googlesource.com",
+ "project",
+ false,
+ },
+ {
+ &buildbucketpb.Build_Input{},
+ "https://fuchsia.googlesource.com/integration",
+ "fuchsia.googlesource.com",
+ "integration",
+ false,
},
}
for _, test := range tests {
- SetGitilesCommit(test.input, test.remote, test.ref)
- if !reflect.DeepEqual(test.input, test.expected) {
- t.Fatalf("wanted:\n%v\ngot:\n%v\n", test.input, test.expected)
+ host, project, err := ResolveRepo(test.input, test.fallbackRemote)
+ if err == nil {
+ if test.expectErr {
+ t.Fatalf("expected error, got nil")
+ }
+ if host != test.expectedHost {
+ t.Fatalf("host %q does not match expected %q", host, test.expectedHost)
+ }
+ if project != test.expectedProject {
+ t.Fatalf("project %q does not match expected %q", project, test.expectedProject)
+ }
+ } else if !test.expectErr {
+ t.Fatalf("got unexpected error %v", err)
}
}
}
@@ -108,44 +157,70 @@
}
}
-func TestIsRecipeChange(t *testing.T) {
+func TestHasRepoChange(t *testing.T) {
t.Parallel()
var tests = []struct {
- change *buildbucketpb.GerritChange
+ input *buildbucketpb.Build_Input
+ remote string
expected bool
}{
{
- &buildbucketpb.GerritChange{
- Host: "fuchsia-review.googlesource.com",
- Project: "infra/recipes",
- Change: 123456,
- Patchset: 1,
+ &buildbucketpb.Build_Input{
+ GerritChanges: []*buildbucketpb.GerritChange{
+ {
+ Host: "fuchsia-review.googlesource.com",
+ Project: "infra/recipes",
+ },
+ },
},
+ "https://fuchsia.googlesource.com/infra/recipes",
true,
},
{
- &buildbucketpb.GerritChange{
- Host: "foo-review.googlesource.com",
- Project: "infra/recipes",
- Change: 123456,
- Patchset: 1,
+ &buildbucketpb.Build_Input{
+ GerritChanges: []*buildbucketpb.GerritChange{
+ {
+ Host: "foo-review.googlesource.com",
+ Project: "infra/recipes",
+ },
+ },
},
+ "https://fuchsia.googlesource.com/infra/recipes",
false,
},
{
- &buildbucketpb.GerritChange{
- Host: "fuchsia-review.googlesource.com",
- Project: "fuchsia",
- Change: 123456,
- Patchset: 1,
+ &buildbucketpb.Build_Input{
+ GerritChanges: []*buildbucketpb.GerritChange{
+ {
+ Host: "fuchsia-review.googlesource.com",
+ Project: "fuchsia",
+ },
+ },
},
+ "https://fuchsia.googlesource.com/infra/recipes",
false,
},
+ {
+ &buildbucketpb.Build_Input{
+ GerritChanges: []*buildbucketpb.GerritChange{
+ {
+ Host: "fuchsia-review.googlesource.com",
+ Project: "fuchsia",
+ },
+ {
+ Host: "fuchsia-review.googlesource.com",
+ Project: "other",
+ },
+ },
+ },
+ "https://fuchsia.googlesource.com/fuchsia",
+ true,
+ },
}
for _, test := range tests {
- ok, err := IsRecipeChange(test.change, "https://fuchsia.googlesource.com/infra/recipes")
+ ok, err := HasRepoChange(test.input, test.remote)
if err != nil {
t.Fatalf("got unexpected error %v", err)
}
diff --git a/cmd/recipe_bootstrap/main.go b/cmd/recipe_bootstrap/main.go
index 095d78c..549cbc5 100644
--- a/cmd/recipe_bootstrap/main.go
+++ b/cmd/recipe_bootstrap/main.go
@@ -23,7 +23,10 @@
rbc "go.fuchsia.dev/infra/cmd/recipe_bootstrap/checkout"
"go.fuchsia.dev/infra/cmd/recipe_bootstrap/manifest"
"go.fuchsia.dev/infra/cmd/recipe_bootstrap/props"
+ "go.fuchsia.dev/infra/gitiles"
+ "go.chromium.org/luci/auth"
+ gitilesapi "go.chromium.org/luci/common/api/gitiles"
"go.chromium.org/luci/common/logging"
"go.chromium.org/luci/common/logging/gologger"
@@ -48,36 +51,61 @@
defaultCheckoutTimeout = 5 * time.Minute
)
-// ensureInput ensures that the incoming build input always has either a
-// GitilesCommit or a GitilesChange.
+// resolveHead returns a resolved sha1 from the HEAD of a repository.
+func resolveHead(ctx context.Context, host, project string) (string, error) {
+ authClient, err := auth.NewAuthenticator(ctx, auth.SilentLogin, auth.Options{Scopes: []string{gitilesapi.OAuthScope}}).Client()
+ if err != nil {
+ return "", fmt.Errorf("could not initialize auth client: %v", err)
+ }
+ client, err := gitiles.NewClient(ctx, host, project, authClient)
+ if err != nil {
+ return "", fmt.Errorf("could not initialize gitiles client: %v", err)
+ }
+ return client.LatestCommit(ctx)
+}
+
+// ensureGitilesCommit ensures that the incoming build always has an
+// Input.GitilesCommit. This ensures that this build and any child builds
+// always use a consistent HEAD.
//
-// If the build input has neither, set Build.Input.GitilesCommit to the
-// integration remote at HEAD. HEAD will be resolved during recipe resolution.
-//
-// Integration remote is either the default set in this file, or the override
-// set by the input property `recipe_integration_remote`.
-func ensureBuildInput(build *buildbucketpb.Build) error {
- if len(build.Input.GerritChanges) > 0 || build.Input.GitilesCommit != nil {
+// The host and project for the GitilesCommit are determined by the
+// `ResolveRepo` strategy, with the fallback remote being either the default
+// integration remote set in this file, or the override set by the
+// `recipe_integration_remote` property.
+func ensureGitilesCommit(ctx context.Context, build *buildbucketpb.Build) error {
+ if build.Input.GitilesCommit != nil {
return nil
}
- remote, err := props.IntegrationRemote(build)
+ fallbackRemote, err := props.IntegrationRemote(build)
if err != nil {
return fmt.Errorf("failed to resolve recipe_integration_remote property: %v", err)
}
- if remote == "" {
- remote = defaultIntegrationRemote
+ if fallbackRemote == "" {
+ fallbackRemote = defaultIntegrationRemote
}
- return rbc.SetGitilesCommit(build.Input, remote, defaultIntegrationRef)
+ host, project, err := rbc.ResolveRepo(build.Input, fallbackRemote)
+ if err != nil {
+ return fmt.Errorf("failed to resolve host and project: %v", err)
+ }
+ revision, err := resolveHead(ctx, host, project)
+ if err != nil {
+ return fmt.Errorf("failed to resolve HEAD: %v", err)
+ }
+ build.Input.GitilesCommit = &buildbucketpb.GitilesCommit{
+ Host: host,
+ Project: project,
+ Id: revision,
+ }
+ return nil
}
-// resolveRecipeVersion resolves the recipe version to use, ensuring that it
-// is set in Build.Input.Properties.
+// resolveRecipeVersion resolves the recipe version to use.
//
-// If the recipe version is not already set in Build.Input.Properties, and the
-// recipes repo is under test, this function sets the recipe version property
-// to the recipes patchset.
+// If Build.Input.GerritChanges indicates that the recipes repo is under test,
+// we short-circuit immediately.
//
-// Otherwise, this function does a checkout and sets the recipe version property.
+// In all other cases, we ensure that the recipe version property is set in
+// Build.Input.Properties.
func resolveRecipeVersion(ctx context.Context, build *buildbucketpb.Build) error {
recipeVersion, err := props.RecipeVersion(build)
if err != nil {
@@ -87,14 +115,14 @@
if recipeVersion != "" {
return nil
}
- // If the recipes repo is under test, set the recipe version to the patchset.
- if len(build.Input.GerritChanges) > 0 {
- ok, err := rbc.IsRecipeChange(build.Input.GerritChanges[0], recipesRemote)
- if err != nil {
- return err
- } else if ok {
- return props.SetRecipeVersion(build, checkout.GitilesChangeRef(*build.Input.GerritChanges[0]))
- }
+ // If the recipes repo is under test, we're done. checkoutRecipes will use
+ // the build input directly.
+ hasRecipeChange, err := rbc.HasRepoChange(build.Input, recipesRemote)
+ if err != nil {
+ return fmt.Errorf("could not determine whether build input has recipe change: %v", err)
+ }
+ if hasRecipeChange {
+ return nil
}
// Otherwise, resolve the recipe version from the checkout.
integrationURL, err := rbc.ResolveIntegrationURL(build.Input)
@@ -129,30 +157,43 @@
return props.SetRecipeVersion(build, proj.Revision)
}
-// checkoutRecipes checks out the recipes.git repo in dir at the revision
-// specified by Build.Input.Properties.
+// checkoutRecipes checks out the recipes.git repo in dir.
+//
+// If the build input has a recipe change, then checkout directly using the
+// build input.
+//
+// Otherwise, checkout at the revision specified by Build.Input.Properties.
func checkoutRecipes(ctx context.Context, dir string, build *buildbucketpb.Build) error {
- recipeVersion, err := props.RecipeVersion(build)
- if err != nil {
- return err
- }
- if recipeVersion == "" {
- return errors.New("recipe_version property is unexpectedly empty")
- }
recipesURL, err := url.Parse(recipesRemote)
if err != nil {
return fmt.Errorf("could not parse URL %s", recipesRemote)
}
- input := buildbucketpb.Build_Input{
- GitilesCommit: &buildbucketpb.GitilesCommit{
- Host: recipesURL.Host,
- Project: strings.TrimLeft(recipesURL.Path, "/"),
- Id: recipeVersion,
- },
+ var input *buildbucketpb.Build_Input
+ hasRecipeChange, err := rbc.HasRepoChange(build.Input, recipesRemote)
+ if err != nil {
+ return fmt.Errorf("could not determine whether build input has recipe change: %v", err)
+ }
+ if hasRecipeChange {
+ input = build.Input
+ } else {
+ recipeVersion, err := props.RecipeVersion(build)
+ if err != nil {
+ return err
+ }
+ if recipeVersion == "" {
+ return errors.New("recipe_version property is unexpectedly empty")
+ }
+ input = &buildbucketpb.Build_Input{
+ GitilesCommit: &buildbucketpb.GitilesCommit{
+ Host: recipesURL.Host,
+ Project: strings.TrimLeft(recipesURL.Path, "/"),
+ Id: recipeVersion,
+ },
+ }
}
tctx, cancel := context.WithTimeout(ctx, defaultCheckoutTimeout)
defer cancel()
- if err := checkout.Checkout(tctx, input, *recipesURL, "", dir); err != nil {
+ if err := checkout.Checkout(tctx, *input, *recipesURL, "", dir); err != nil {
return fmt.Errorf("failed to checkout: %v", err)
}
return nil
@@ -188,8 +229,8 @@
if err := proto.Unmarshal(data, build); err != nil {
fail(ctx, fmt.Sprintf("failed to unmarshal build.proto from stdin: %v", err))
}
- if err := ensureBuildInput(build); err != nil {
- fail(ctx, fmt.Sprintf("failed to ensure build has input: %v", err))
+ if err := ensureGitilesCommit(ctx, build); err != nil {
+ fail(ctx, fmt.Sprintf("failed to ensure that build has a gitiles commit: %v", err))
}
if err := resolveRecipeVersion(ctx, build); err != nil {
fail(ctx, fmt.Sprintf("failed to resolve recipe version: %v", err))
