From 20032673ea762015e9d3002f08dfad02525fc97c Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Mon, 9 Nov 2020 13:17:35 -0700 Subject: [PATCH 1/3] feat(internal/gapicgen): support adding context to regen Now all regen PRs will include a block that mentions all of the changes that happened in googleapis/googleapis that caused the files to be generated. This information will eventually be fed into our automated releases via release-please. There these changes will be parsed out to add extra context to our release PRs and changelogs. No longer will we have to have a vague message about many auto-generated changes. - Adds context to regen PRs. - Refactored generators so every method does not need so many parameters. - Updated goolge/protobuf to its new migrated repo protocolbuffers/protobuf. - Added some extra logging to the build. - Only run protoc on dirs that have changes. --- .gitignore | 1 + internal/gapicgen/README.md | 2 +- internal/gapicgen/cmd/genbot/README.md | 9 +- internal/gapicgen/cmd/genbot/generate.go | 17 +-- internal/gapicgen/cmd/genbot/github.go | 76 ++++++++--- internal/gapicgen/cmd/genlocal/main.go | 19 ++- internal/gapicgen/generator/config.go | 20 +++ internal/gapicgen/generator/gapics.go | 95 ++++++++------ internal/gapicgen/generator/generator.go | 70 ++++++---- internal/gapicgen/generator/genproto.go | 155 ++++++++++++----------- internal/gapicgen/generator/git.go | 138 ++++++++++++++++++++ 11 files changed, 427 insertions(+), 175 deletions(-) create mode 100644 internal/gapicgen/generator/git.go diff --git a/.gitignore b/.gitignore index ee9694b8780..cc7e53b46c0 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,7 @@ .idea .vscode *.swp +.history # Test files *.test diff --git a/internal/gapicgen/README.md b/internal/gapicgen/README.md index 1e696fa08e9..fdf9f2b9ed1 100644 --- a/internal/gapicgen/README.md +++ b/internal/gapicgen/README.md @@ -13,4 +13,4 @@ gapicgen contains three binaries: gapic regen CL that needs to have reviewers added and go.mod update, and then does so. Intended to be run periodically as a bot, but humans can use it too. -See the README.md in each folder for more specific instructions. \ No newline at end of file +See the README.md in each folder for more specific instructions. diff --git a/internal/gapicgen/cmd/genbot/README.md b/internal/gapicgen/cmd/genbot/README.md index 1fa64ea63bf..5bdafc8d213 100644 --- a/internal/gapicgen/cmd/genbot/README.md +++ b/internal/gapicgen/cmd/genbot/README.md @@ -7,8 +7,9 @@ It is intended to be used as a bot, though it can be run locally too. ### Github -For Github, you need to generate/supply a Personal Access Token. More information on how that's done is here: -https://help.github.com/en/github/authenticating-to-github/creating-a-personal-access-token-for-the-command-line +For Github, you need to generate/supply a Personal Access Token. More +information on how that's done can be found here: +[creating a personal access token](https://help.github.com/en/github/authenticating-to-github/creating-a-personal-access-token-for-the-command-line). ## Running locally @@ -48,9 +49,9 @@ docker run -t --rm --privileged \ ## FAQ -#### How do I bump to a later version of the microgenerator? +### How to bump to a later version of the microgenerator -``` +```shell cd /path/to/internal/gapicgen go get -u github.com/googleapis/gapic-generator-go/cmd/protoc-gen-go_gapic ``` diff --git a/internal/gapicgen/cmd/genbot/generate.go b/internal/gapicgen/cmd/genbot/generate.go index bdb96f17d3c..d53740ffbd5 100644 --- a/internal/gapicgen/cmd/genbot/generate.go +++ b/internal/gapicgen/cmd/genbot/generate.go @@ -60,14 +60,15 @@ func generate(ctx context.Context, githubClient *GithubClient) error { return gitClone("https://github.com/googleapis/google-cloud-go", gocloudDir) }) grp.Go(func() error { - return gitClone("https://github.com/google/protobuf", protoDir) + return gitClone("https://github.com/protocolbuffers/protobuf", protoDir) }) if err := grp.Wait(); err != nil { log.Println(err) } // Regen. - if err := generator.Generate(ctx, googleapisDir, genprotoDir, gocloudDir, protoDir, ""); err != nil { + changes, err := generator.Generate(ctx, googleapisDir, genprotoDir, gocloudDir, protoDir, "") + if err != nil { return err } @@ -85,17 +86,17 @@ func generate(ctx context.Context, githubClient *GithubClient) error { switch { case genprotoHasChanges && gocloudHasChanges: // Both have changes. - genprotoPRNum, err := githubClient.CreateGenprotoPR(ctx, genprotoDir, true) + genprotoPRNum, err := githubClient.CreateGenprotoPR(ctx, genprotoDir, true, changes) if err != nil { return fmt.Errorf("error creating PR for genproto (may need to check logs for more errors): %v", err) } - gocloudPRNum, err := githubClient.CreateGocloudPR(ctx, gocloudDir, genprotoPRNum) + gocloudPRNum, err := githubClient.CreateGocloudPR(ctx, gocloudDir, genprotoPRNum, changes) if err != nil { return fmt.Errorf("error creating CL for veneers (may need to check logs for more errors): %v", err) } - if err := githubClient.AmendWithPRURL(ctx, genprotoPRNum, genprotoDir, gocloudPRNum); err != nil { + if err := githubClient.AmendGenprotoPR(ctx, genprotoPRNum, genprotoDir, gocloudPRNum, changes); err != nil { return fmt.Errorf("error amending genproto PR: %v", err) } @@ -105,7 +106,7 @@ func generate(ctx context.Context, githubClient *GithubClient) error { log.Println(gocloudPRURL) case genprotoHasChanges: // Only genproto has changes. - genprotoPRNum, err := githubClient.CreateGenprotoPR(ctx, genprotoDir, false) + genprotoPRNum, err := githubClient.CreateGenprotoPR(ctx, genprotoDir, false, changes) if err != nil { return fmt.Errorf("error creating PR for genproto (may need to check logs for more errors): %v", err) } @@ -115,7 +116,7 @@ func generate(ctx context.Context, githubClient *GithubClient) error { log.Println("gocloud had no changes") case gocloudHasChanges: // Only gocloud has changes. - gocloudPRNum, err := githubClient.CreateGocloudPR(ctx, gocloudDir, -1) + gocloudPRNum, err := githubClient.CreateGocloudPR(ctx, gocloudDir, -1, changes) if err != nil { return fmt.Errorf("error creating CL for veneers (may need to check logs for more errors): %v", err) } @@ -145,7 +146,7 @@ func gitClone(repo, dir string) error { func hasChanges(dir string) (bool, error) { // Write command output to both os.Stderr and local, so that we can check // whether there are modified files. - inmem := bytes.NewBuffer([]byte{}) // TODO(deklerk): Try `var inmem bytes.Buffer`. + inmem := bytes.NewBuffer(nil) w := io.MultiWriter(os.Stderr, inmem) c := exec.Command("bash", "-c", "git status --short") diff --git a/internal/gapicgen/cmd/genbot/github.go b/internal/gapicgen/cmd/genbot/github.go index d9bede8c481..fb7a9d3e3ee 100644 --- a/internal/gapicgen/cmd/genbot/github.go +++ b/internal/gapicgen/cmd/genbot/github.go @@ -26,6 +26,7 @@ import ( "strings" "time" + "cloud.google.com/go/internal/gapicgen/generator" "github.com/google/go-github/v32/github" "github.com/shurcooL/githubv4" "golang.org/x/oauth2" @@ -178,13 +179,15 @@ func (gc *GithubClient) GetRegenPR(ctx context.Context, repo string, status stri // CreateGenprotoPR creates a PR for a given genproto change. // // hasCorrespondingPR indicates that there is a corresponding google-cloud-go PR. -func (gc *GithubClient) CreateGenprotoPR(ctx context.Context, genprotoDir string, hasCorrespondingPR bool) (prNumber int, _ error) { +func (gc *GithubClient) CreateGenprotoPR(ctx context.Context, genprotoDir string, hasCorrespondingPR bool, changes []*generator.ChangeInfo) (prNumber int, _ error) { log.Println("creating genproto PR") - - body := genprotoCommitBody + var body strings.Builder + body.WriteString(genprotoCommitBody) if !hasCorrespondingPR { - body += "\n\nThere is no corresponding google-cloud-go PR.\n" + body.WriteString("\n\nThere is no corresponding google-cloud-go PR.\n") + body.WriteString(formatChanges(changes, false)) } + sBody := body.String() c := exec.Command("/bin/bash", "-c", ` set -ex @@ -206,7 +209,7 @@ git push origin $BRANCH_NAME fmt.Sprintf("PATH=%s", os.Getenv("PATH")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. fmt.Sprintf("HOME=%s", os.Getenv("HOME")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. fmt.Sprintf("COMMIT_TITLE=%s", genprotoCommitTitle), - fmt.Sprintf("COMMIT_BODY=%s", body), + fmt.Sprintf("COMMIT_BODY=%s", sBody), fmt.Sprintf("BRANCH_NAME=%s", genprotoBranchName), } c.Dir = genprotoDir @@ -219,9 +222,10 @@ git push origin $BRANCH_NAME t := genprotoCommitTitle // Because we have to take the address. pr, _, err := gc.cV3.PullRequests.Create(ctx, "googleapis", "go-genproto", &github.NewPullRequest{ Title: &t, - Body: &body, + Body: &sBody, Head: &head, Base: &base, + Draft: github.Bool(true), }) if err != nil { return 0, err @@ -246,18 +250,21 @@ git push origin $BRANCH_NAME return pr.GetNumber(), nil } -// CreateGocloudPR creats a PR for a given google-cloud-go change. -func (gc *GithubClient) CreateGocloudPR(ctx context.Context, gocloudDir string, genprotoPRNum int) (prNumber int, _ error) { +// CreateGocloudPR creates a PR for a given google-cloud-go change. +func (gc *GithubClient) CreateGocloudPR(ctx context.Context, gocloudDir string, genprotoPRNum int, changes []*generator.ChangeInfo) (prNumber int, _ error) { log.Println("creating google-cloud-go PR") - var body string + body := &strings.Builder{} var draft bool + body.WriteString(gocloudCommitBody) if genprotoPRNum > 0 { - body = gocloudCommitBody + fmt.Sprintf("\n\nCorresponding genproto PR: https://github.com/googleapis/go-genproto/pull/%d\n", genprotoPRNum) + body.WriteString(fmt.Sprintf("\n\nCorresponding genproto PR: https://github.com/googleapis/go-genproto/pull/%d\n", genprotoPRNum)) draft = true } else { - body = gocloudCommitBody + "\n\nThere is no corresponding genproto PR.\n" + body.WriteString("\n\nThere is no corresponding genproto PR.\n") } + body.WriteString(formatChanges(changes, true)) + sBody := body.String() c := exec.Command("/bin/bash", "-c", ` set -ex @@ -279,7 +286,7 @@ git push origin $BRANCH_NAME fmt.Sprintf("PATH=%s", os.Getenv("PATH")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. fmt.Sprintf("HOME=%s", os.Getenv("HOME")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. fmt.Sprintf("COMMIT_TITLE=%s", gocloudCommitTitle), - fmt.Sprintf("COMMIT_BODY=%s", body), + fmt.Sprintf("COMMIT_BODY=%s", sBody), fmt.Sprintf("BRANCH_NAME=%s", gocloudBranchName), } c.Dir = gocloudDir @@ -290,7 +297,7 @@ git push origin $BRANCH_NAME t := gocloudCommitTitle // Because we have to take the address. pr, _, err := gc.cV3.PullRequests.Create(ctx, "googleapis", "google-cloud-go", &github.NewPullRequest{ Title: &t, - Body: &body, + Body: &sBody, Head: github.String(fmt.Sprintf("googleapis:" + gocloudBranchName)), Base: github.String("master"), Draft: github.Bool(draft), @@ -304,11 +311,14 @@ git push origin $BRANCH_NAME return pr.GetNumber(), nil } -// AmendWithPRURL amends the given genproto PR with a link to the given +// AmendGenprotoPR amends the given genproto PR with a link to the given // google-cloud-go PR. -func (gc *GithubClient) AmendWithPRURL(ctx context.Context, genprotoPRNum int, genprotoDir string, gocloudPRNum int) error { - newBody := genprotoCommitBody + fmt.Sprintf("\n\nCorresponding google-cloud-go PR: googleapis/google-cloud-go#%d\n", gocloudPRNum) - +func (gc *GithubClient) AmendGenprotoPR(ctx context.Context, genprotoPRNum int, genprotoDir string, gocloudPRNum int, changes []*generator.ChangeInfo) error { + var body strings.Builder + body.WriteString(genprotoCommitBody) + body.WriteString(fmt.Sprintf("\n\nCorresponding google-cloud-go PR: googleapis/google-cloud-go#%d\n", gocloudPRNum)) + body.WriteString(formatChanges(changes, false)) + sBody := body.String() c := exec.Command("/bin/bash", "-c", ` set -ex @@ -325,7 +335,7 @@ git push -f origin $BRANCH_NAME fmt.Sprintf("PATH=%s", os.Getenv("PATH")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. fmt.Sprintf("HOME=%s", os.Getenv("HOME")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. fmt.Sprintf("COMMIT_TITLE=%s", genprotoCommitTitle), - fmt.Sprintf("COMMIT_BODY=%s", newBody), + fmt.Sprintf("COMMIT_BODY=%s", sBody), fmt.Sprintf("BRANCH_NAME=%s", genprotoBranchName), } c.Dir = genprotoDir @@ -333,7 +343,7 @@ git push -f origin $BRANCH_NAME return err } _, _, err := gc.cV3.PullRequests.Edit(ctx, "googleapis", "go-genproto", genprotoPRNum, &github.PullRequest{ - Body: &newBody, + Body: &sBody, }) return err } @@ -356,3 +366,31 @@ func (gc *GithubClient) MarkPRReadyForReview(ctx context.Context, repo string, n } return nil } + +func formatChanges(changes []*generator.ChangeInfo, onlyGapicChanges bool) string { + if len(changes) == 0 { + return "" + } + var sb strings.Builder + sb.WriteString("\nChanges:\n") + for _, c := range changes { + if onlyGapicChanges && !c.HasGapicChanges { + continue + } + sb.WriteString("- ") + ss := strings.Split(c.Body, "\n") + for i, s := range ss { + if i == 0 { + sb.WriteString(fmt.Sprintf("%s\n", s)) + continue + } + if s == "" { + sb.WriteString("\n") + continue + } + sb.WriteString(fmt.Sprintf(" %s\n", s)) + } + sb.WriteString("\n") + } + return sb.String() +} diff --git a/internal/gapicgen/cmd/genlocal/main.go b/internal/gapicgen/cmd/genlocal/main.go index dfd97f3102d..98cfd3fa557 100644 --- a/internal/gapicgen/cmd/genlocal/main.go +++ b/internal/gapicgen/cmd/genlocal/main.go @@ -20,6 +20,7 @@ package main import ( "context" "flag" + "fmt" "io/ioutil" "log" "os" @@ -54,32 +55,40 @@ func main() { genprotoDir := flag.String("genproto-dir", filepath.Join(tmpDir, "genproto"), "Directory where sources of googleapis/go-genproto resides. If unset the sources will be cloned to a temporary directory that is not cleaned up.") protoDir := flag.String("proto-dir", filepath.Join(tmpDir, "proto"), "Directory where sources of google/protobuf resides. If unset the sources will be cloned to a temporary directory that is not cleaned up.") gapicToGenerate := flag.String("gapic", "", `Specifies which gapic to generate. The value should be in the form of an import path (Ex: cloud.google.com/go/pubsub/apiv1). The default "" generates all gapics.`) + verbose := flag.Bool("verbose", false, "Enables verbose logging.") flag.Parse() ctx := context.Background() // Clone repositories if needed. - grp, _ := errgroup.WithContext(ctx) gitClone(grp, "https://github.com/googleapis/googleapis.git", *googleapisDir, tmpDir) gitClone(grp, "https://github.com/googleapis/go-genproto", *genprotoDir, tmpDir) gitClone(grp, "https://github.com/googleapis/google-cloud-go", *gocloudDir, tmpDir) - gitClone(grp, "https://github.com/google/protobuf", *protoDir, tmpDir) + gitClone(grp, "https://github.com/protocolbuffers/protobuf", *protoDir, tmpDir) if err := grp.Wait(); err != nil { log.Println(err) } // Regen. - - if err := generator.Generate(ctx, *googleapisDir, *genprotoDir, *gocloudDir, *protoDir, *gapicToGenerate); err != nil { + changes, err := generator.Generate(ctx, *googleapisDir, *genprotoDir, *gocloudDir, *protoDir, *gapicToGenerate) + if err != nil { log.Printf("Generator ran (and failed) in %s\n", tmpDir) log.Fatal(err) } // Log results. - log.Println(genprotoDir) log.Println(gocloudDir) + + if *verbose { + log.Println("Changes:") + fmt.Println() + for _, v := range changes { + fmt.Println("********************************************") + fmt.Println(v.Body) + } + } } // gitClone clones a repository in the given directory if dir is not in tmpDir. diff --git a/internal/gapicgen/generator/config.go b/internal/gapicgen/generator/config.go index ad3d90b251f..dfa72f8ea77 100644 --- a/internal/gapicgen/generator/config.go +++ b/internal/gapicgen/generator/config.go @@ -14,6 +14,26 @@ package generator +import "strings" + +// gapicPkgs is a map of googleapis inputDirectoryPath to the gapic package name +// used for conventional commits. +var gapicPkgs map[string]string + +func init() { + gapicPkgs = make(map[string]string) + for _, v := range microgenGapicConfigs { + gapicPkgs[v.inputDirectoryPath] = parseConventionalCommitPkg(v.importPath) + } +} + +func parseConventionalCommitPkg(importPath string) string { + s := strings.TrimPrefix(importPath, "cloud.google.com/go/") + ss := strings.Split(s, "/") + // remove the version, i.e /apiv1 + return strings.Join(ss[:len(ss)-1], "/") +} + // microgenConfig represents a single microgen target. type microgenConfig struct { // inputDirectoryPath is the path to the input (.proto, etc) files, relative diff --git a/internal/gapicgen/generator/gapics.go b/internal/gapicgen/generator/gapics.go index ebf57aa9522..4d960b0bd70 100644 --- a/internal/gapicgen/generator/gapics.go +++ b/internal/gapicgen/generator/gapics.go @@ -26,8 +26,27 @@ import ( "gopkg.in/yaml.v2" ) -// generateGapics generates gapics. -func generateGapics(ctx context.Context, googleapisDir, protoDir, gocloudDir, genprotoDir string, gapicToGenerate string) error { +// GapicGenerator is used to regenerate gapic libraries. +type GapicGenerator struct { + googleapisDir string + protoDir string + googleCloudDir string + genprotoDir string +} + +// NewGapicGenerator creates a GapicGenerator. +func NewGapicGenerator(googleapisDir, protoDir, googleCloudDir, genprotoDir string) *GapicGenerator { + return &GapicGenerator{ + googleapisDir: googleapisDir, + protoDir: protoDir, + googleCloudDir: googleCloudDir, + genprotoDir: genprotoDir, + } +} + +// Regen generates gapics. +func (g *GapicGenerator) Regen(ctx context.Context, gapicToGenerate string) error { + log.Println("regenerating gapics") for _, c := range microgenGapicConfigs { // Skip generation if generating all of the gapics and the associated // config has a block on it. Or if generating a single gapic and it does @@ -36,36 +55,36 @@ func generateGapics(ctx context.Context, googleapisDir, protoDir, gocloudDir, ge (gapicToGenerate != "" && gapicToGenerate != c.importPath) { continue } - if err := microgen(c, googleapisDir, protoDir, gocloudDir); err != nil { + if err := g.microgen(c); err != nil { return err } } - if err := copyMicrogenFiles(gocloudDir); err != nil { + if err := g.copyMicrogenFiles(); err != nil { return err } - if err := manifest(microgenGapicConfigs, googleapisDir, gocloudDir); err != nil { + if err := g.manifest(microgenGapicConfigs); err != nil { return err } - if err := setVersion(gocloudDir); err != nil { + if err := g.setVersion(); err != nil { return err } - if err := addModReplaceGenproto(gocloudDir, genprotoDir); err != nil { + if err := g.addModReplaceGenproto(); err != nil { return err } - if err := vet(gocloudDir); err != nil { + if err := vet(g.googleCloudDir); err != nil { return err } - if err := build(gocloudDir); err != nil { + if err := build(g.googleCloudDir); err != nil { return err } - if err := dropModReplaceGenproto(gocloudDir); err != nil { + if err := g.dropModReplaceGenproto(); err != nil { return err } @@ -75,18 +94,17 @@ func generateGapics(ctx context.Context, googleapisDir, protoDir, gocloudDir, ge // addModReplaceGenproto adds a genproto replace statement that points genproto // to the local copy. This is necessary since the remote genproto may not have // changes that are necessary for the in-flight regen. -func addModReplaceGenproto(gocloudDir, genprotoDir string) error { +func (g *GapicGenerator) addModReplaceGenproto() error { + log.Println("adding temporary genproto replace statement") c := command("bash", "-c", ` set -ex GENPROTO_VERSION=$(cat go.mod | cat go.mod | grep genproto | awk '{print $2}') go mod edit -replace "google.golang.org/genproto@$GENPROTO_VERSION=$GENPROTO_DIR" `) - c.Stdout = os.Stdout - c.Stderr = os.Stderr - c.Dir = gocloudDir + c.Dir = g.googleCloudDir c.Env = []string{ - "GENPROTO_DIR=" + genprotoDir, + "GENPROTO_DIR=" + g.genprotoDir, fmt.Sprintf("PATH=%s", os.Getenv("PATH")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. fmt.Sprintf("HOME=%s", os.Getenv("HOME")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. } @@ -95,16 +113,15 @@ go mod edit -replace "google.golang.org/genproto@$GENPROTO_VERSION=$GENPROTO_DIR // dropModReplaceGenproto drops the genproto replace statement. It is intended // to be run after addModReplaceGenproto. -func dropModReplaceGenproto(gocloudDir string) error { +func (g *GapicGenerator) dropModReplaceGenproto() error { + log.Println("removing genproto replace statement") c := command("bash", "-c", ` set -ex GENPROTO_VERSION=$(cat go.mod | cat go.mod | grep genproto | grep -v replace | awk '{print $2}') go mod edit -dropreplace "google.golang.org/genproto@$GENPROTO_VERSION" `) - c.Stdout = os.Stdout - c.Stderr = os.Stderr - c.Dir = gocloudDir + c.Dir = g.googleCloudDir c.Env = []string{ fmt.Sprintf("PATH=%s", os.Getenv("PATH")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. fmt.Sprintf("HOME=%s", os.Getenv("HOME")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. @@ -115,7 +132,8 @@ go mod edit -dropreplace "google.golang.org/genproto@$GENPROTO_VERSION" // setVersion updates the versionClient constant in all .go files. It may create // .backup files on certain systems (darwin), and so should be followed by a // clean-up of .backup files. -func setVersion(gocloudDir string) error { +func (g *GapicGenerator) setVersion() error { + log.Println("updating client version") // TODO(deklerk): Migrate this all to Go instead of using bash. c := command("bash", "-c", ` @@ -126,18 +144,16 @@ git ls-files -mo | while read modified; do done find . -name '*.backup' -delete `) - c.Stdout = os.Stdout - c.Stderr = os.Stderr - c.Dir = gocloudDir + c.Dir = g.googleCloudDir return c.Run() } // microgen runs the microgenerator on a single microgen config. -func microgen(conf *microgenConfig, googleapisDir, protoDir, gocloudDir string) error { +func (g *GapicGenerator) microgen(conf *microgenConfig) error { log.Println("microgen generating", conf.pkg) var protoFiles []string - if err := filepath.Walk(googleapisDir+"/"+conf.inputDirectoryPath, func(path string, info os.FileInfo, err error) error { + if err := filepath.Walk(g.googleapisDir+"/"+conf.inputDirectoryPath, func(path string, info os.FileInfo, err error) error { if err != nil { return err } @@ -149,19 +165,17 @@ func microgen(conf *microgenConfig, googleapisDir, protoDir, gocloudDir string) return err } - args := []string{"-I", googleapisDir, + args := []string{"-I", g.googleapisDir, "--experimental_allow_proto3_optional", - "-I", protoDir, - "--go_gapic_out", gocloudDir, + "-I", g.protoDir, + "--go_gapic_out", g.googleCloudDir, "--go_gapic_opt", fmt.Sprintf("go-gapic-package=%s;%s", conf.importPath, conf.pkg), "--go_gapic_opt", fmt.Sprintf("grpc-service-config=%s", conf.gRPCServiceConfigPath), "--go_gapic_opt", fmt.Sprintf("gapic-service-config=%s", conf.apiServiceConfigPath), "--go_gapic_opt", fmt.Sprintf("release-level=%s", conf.releaseLevel)} args = append(args, protoFiles...) c := command("protoc", args...) - c.Stdout = os.Stdout - c.Stderr = os.Stderr - c.Dir = googleapisDir + c.Dir = g.googleapisDir return c.Run() } @@ -286,9 +300,10 @@ var manualEntries = []manifestEntry{ } // manifest writes a manifest file with info about all of the confs. -func manifest(confs []*microgenConfig, googleapisDir, gocloudDir string) error { +func (g *GapicGenerator) manifest(confs []*microgenConfig) error { + log.Println("updating gapic manifest") entries := map[string]manifestEntry{} // Key is the package name. - f, err := os.Create(filepath.Join(gocloudDir, "internal", ".repo-metadata-full.json")) + f, err := os.Create(filepath.Join(g.googleCloudDir, "internal", ".repo-metadata-full.json")) if err != nil { return err } @@ -297,7 +312,7 @@ func manifest(confs []*microgenConfig, googleapisDir, gocloudDir string) error { entries[manual.DistributionName] = manual } for _, conf := range confs { - yamlPath := filepath.Join(googleapisDir, conf.apiServiceConfigPath) + yamlPath := filepath.Join(g.googleapisDir, conf.apiServiceConfigPath) yamlFile, err := os.Open(yamlPath) if err != nil { return err @@ -325,19 +340,15 @@ func manifest(confs []*microgenConfig, googleapisDir, gocloudDir string) error { // copyMicrogenFiles takes microgen files from gocloudDir/cloud.google.com/go // and places them in gocloudDir. -func copyMicrogenFiles(gocloudDir string) error { +func (g *GapicGenerator) copyMicrogenFiles() error { // The period at the end is analagous to * (copy everything in this dir). - c := command("cp", "-R", gocloudDir+"/cloud.google.com/go/.", ".") - c.Stdout = os.Stdout - c.Stderr = os.Stderr - c.Dir = gocloudDir + c := command("cp", "-R", g.googleCloudDir+"/cloud.google.com/go/.", ".") + c.Dir = g.googleCloudDir if err := c.Run(); err != nil { return err } c = command("rm", "-rf", "cloud.google.com") - c.Stdout = os.Stdout - c.Stderr = os.Stderr - c.Dir = gocloudDir + c.Dir = g.googleCloudDir return c.Run() } diff --git a/internal/gapicgen/generator/generator.go b/internal/gapicgen/generator/generator.go index f1bab52f587..14d2accc418 100644 --- a/internal/gapicgen/generator/generator.go +++ b/internal/gapicgen/generator/generator.go @@ -16,9 +16,9 @@ package generator import ( - "bytes" "context" "fmt" + "io/ioutil" "log" "os" "os/exec" @@ -27,41 +27,64 @@ import ( ) // Generate generates genproto and gapics. -func Generate(ctx context.Context, googleapisDir, genprotoDir, gocloudDir, protoDir string, gapicToGenerate string) error { - if err := regenGenproto(ctx, genprotoDir, googleapisDir, protoDir); err != nil { - return fmt.Errorf("error generating genproto (may need to check logs for more errors): %v", err) +func Generate(ctx context.Context, googleapisDir, genprotoDir, gocloudDir, protoDir string, gapicToGenerate string) ([]*ChangeInfo, error) { + protoGenerator := NewGenprotoGenerator(genprotoDir, googleapisDir, protoDir) + gapicGenerator := NewGapicGenerator(googleapisDir, protoDir, gocloudDir, genprotoDir) + if err := protoGenerator.Regen(ctx); err != nil { + return nil, fmt.Errorf("error generating genproto (may need to check logs for more errors): %v", err) + } + if err := gapicGenerator.Regen(ctx, gapicToGenerate); err != nil { + return nil, fmt.Errorf("error generating gapics (may need to check logs for more errors): %v", err) } - if err := generateGapics(ctx, googleapisDir, protoDir, gocloudDir, genprotoDir, gapicToGenerate); err != nil { - return fmt.Errorf("error generating gapics (may need to check logs for more errors): %v", err) + changes, err := gatherChanges(googleapisDir, genprotoDir) + if err != nil { + return nil, fmt.Errorf("error gathering commit info") } if err := recordGoogleapisHash(googleapisDir, genprotoDir); err != nil { - return fmt.Errorf("error recording most recent googleapis hash: %v", err) + return nil, err } - return nil + return changes, nil +} + +func gatherChanges(googleapisDir, genprotoDir string) ([]*ChangeInfo, error) { + // Get the last processed googleapis hash. + lastHash, err := ioutil.ReadFile(filepath.Join(genprotoDir, "regen.txt")) + if err != nil { + return nil, err + } + commits, err := CommitsSinceHash(googleapisDir, string(lastHash), true) + if err != nil { + return nil, err + } + var changes []*ChangeInfo + for _, v := range commits { + c, err := ParseChangeInfo(googleapisDir, v) + if err != nil { + return nil, err + } + changes = append(changes, c) + } + + return changes, nil } // recordGoogleapisHash parses the latest commit in googleapis and records it to // regen.txt in go-genproto. func recordGoogleapisHash(googleapisDir, genprotoDir string) error { - out := bytes.NewBuffer(nil) - c := command("git", "rev-list", "HEAD^..") - c.Stdout = out - c.Stderr = os.Stderr - c.Dir = googleapisDir - if err := c.Run(); err != nil { + commits, err := CommitsSinceHash(googleapisDir, "HEAD", true) + if err != nil { return err } - commits := strings.Split(strings.TrimSpace(out.String()), "\n") if len(commits) != 1 { return fmt.Errorf("only expected one commit, got %d", len(commits)) } f, err := os.OpenFile(filepath.Join(genprotoDir, "regen.txt"), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644) if err != nil { - return err + log.Fatal(err) } defer f.Close() if _, err := f.WriteString(commits[0]); err != nil { @@ -72,26 +95,22 @@ func recordGoogleapisHash(googleapisDir, genprotoDir string) error { // build attempts to build all packages recursively from the given directory. func build(dir string) error { + log.Println("building generated code") c := command("go", "build", "./...") - c.Stdout = os.Stdout - c.Stderr = os.Stderr c.Dir = dir return c.Run() } // vet runs linters on all .go files recursively from the given directory. func vet(dir string) error { + log.Println("vetting generated code") c := command("goimports", "-w", ".") - c.Stdout = os.Stdout - c.Stderr = os.Stderr c.Dir = dir if err := c.Run(); err != nil { return err } c = command("gofmt", "-s", "-d", "-w", "-l", ".") - c.Stdout = os.Stdout - c.Stderr = os.Stderr c.Dir = dir return c.Run() } @@ -100,8 +119,13 @@ type cmdWrapper struct { *exec.Cmd } +// command wraps a exec.Command to add some logging about commands being run. +// The commands stdout/stderr default to os.Stdout/os.Stderr respectfully. func command(name string, arg ...string) *cmdWrapper { - return &cmdWrapper{exec.Command(name, arg...)} + c := &cmdWrapper{exec.Command(name, arg...)} + c.Stdout = os.Stdout + c.Stderr = os.Stderr + return c } func (cw *cmdWrapper) Run() error { diff --git a/internal/gapicgen/generator/genproto.go b/internal/gapicgen/generator/genproto.go index 2c28a7ab861..0df52ddb8fa 100644 --- a/internal/gapicgen/generator/genproto.go +++ b/internal/gapicgen/generator/genproto.go @@ -20,7 +20,6 @@ import ( "fmt" "io/ioutil" "log" - "os" "path/filepath" "regexp" "strconv" @@ -42,7 +41,23 @@ var denylist = map[string]bool{ "google.golang.org/genproto/googleapis/devtools/containeranalysis/v1": true, } -// regenGenproto regenerates the genproto repository. +// GenprotoGenerator is used to generate code for googleapis/go-genproto. +type GenprotoGenerator struct { + genprotoDir string + googleapisDir string + protoSrcDir string +} + +// NewGenprotoGenerator creates a new GenprotoGenerator. +func NewGenprotoGenerator(genprotoDir, googleapisDir, protoDir string) *GenprotoGenerator { + return &GenprotoGenerator{ + genprotoDir: genprotoDir, + googleapisDir: googleapisDir, + protoSrcDir: filepath.Join(protoDir, "/src"), + } +} + +// Regen regenerates the genproto repository. // // regenGenproto recursively walks through each directory named by given // arguments, looking for all .proto files. (Symlinks are not followed.) Any @@ -55,101 +70,56 @@ var denylist = map[string]bool{ // // Protoc is executed on remaining files, one invocation per set of files // declaring the same Go package. -func regenGenproto(ctx context.Context, genprotoDir, googleapisDir, protoDir string) error { +func (g *GenprotoGenerator) Regen(ctx context.Context) error { log.Println("regenerating genproto") - // The protoc include directory is actually the "src" directory of the repo. - protoDir += "/src" - // Create space to put generated .pb.go's. c := command("mkdir", "generated") - c.Stdout = os.Stdout - c.Stderr = os.Stderr - c.Dir = genprotoDir + c.Dir = g.genprotoDir if err := c.Run(); err != nil { return err } - // Record and map all .proto files to their Go packages. - seenFiles := make(map[string]bool) - pkgFiles := make(map[string][]string) - for _, root := range []string{googleapisDir} { - walkFn := func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - if !info.Mode().IsRegular() || !strings.HasSuffix(path, ".proto") { - return nil - } - - switch rel, err := filepath.Rel(root, path); { - case err != nil: - return err - case seenFiles[rel]: - return nil - default: - seenFiles[rel] = true - } - - pkg, err := goPkg(path) - if err != nil { - return err - } - pkgFiles[pkg] = append(pkgFiles[pkg], path) - return nil - } - if err := filepath.Walk(root, walkFn); err != nil { - return err - } + // Get the last processed googleapis hash. + lastHash, err := ioutil.ReadFile(filepath.Join(g.genprotoDir, "regen.txt")) + if err != nil { + return err } + pkgFiles, err := g.getUpdatedPackages(string(lastHash)) + if err != nil { + return err + } if len(pkgFiles) == 0 { return errors.New("couldn't find any pkgfiles") } - // Run protoc on all protos of all packages. + log.Println("generating from protos") grp, _ := errgroup.WithContext(ctx) - for pkg, fnames := range pkgFiles { + for pkg, fileNames := range pkgFiles { if !strings.HasPrefix(pkg, "google.golang.org/genproto") || denylist[pkg] { continue } pk := pkg - fn := fnames + fn := fileNames grp.Go(func() error { log.Println("running protoc on", pk) - return protoc(genprotoDir, googleapisDir, protoDir, fn) + return g.protoc(fn) }) } if err := grp.Wait(); err != nil { return err } - // Move all generated content to their correct locations in the repository, - // because protoc puts it in a folder called generated/. - - // The period at the end is analagous to * (copy everything in this dir). - c = command("cp", "-R", "generated/google.golang.org/genproto/.", ".") - c.Stdout = os.Stdout - c.Stderr = os.Stderr - c.Dir = genprotoDir - if err := c.Run(); err != nil { + if err := g.moveAndCleanupGeneratedSrc(); err != nil { return err } - c = command("rm", "-rf", "generated") - c.Stdout = os.Stdout - c.Stderr = os.Stderr - c.Dir = genprotoDir - if err := c.Run(); err != nil { + if err := vet(g.genprotoDir); err != nil { return err } - // Clean up and check it all compiles. - if err := vet(genprotoDir); err != nil { - return err - } - - if err := build(genprotoDir); err != nil { + if err := build(g.genprotoDir); err != nil { return err } @@ -158,8 +128,8 @@ func regenGenproto(ctx context.Context, genprotoDir, googleapisDir, protoDir str // goPkg reports the import path declared in the given file's `go_package` // option. If the option is missing, goPkg returns empty string. -func goPkg(fname string) (string, error) { - content, err := ioutil.ReadFile(fname) +func goPkg(fileName string) (string, error) { + content, err := ioutil.ReadFile(fileName) if err != nil { return "", err } @@ -178,14 +148,53 @@ func goPkg(fname string) (string, error) { return pkgName, nil } -// protoc executes the "protoc" command on files named in fnames, and outputs +// protoc executes the "protoc" command on files named in fileNames, and outputs // to "/generated". -func protoc(genprotoDir, googleapisDir, protoDir string, fnames []string) error { - args := []string{"--experimental_allow_proto3_optional", fmt.Sprintf("--go_out=plugins=grpc:%s/generated", genprotoDir), "-I", googleapisDir, "-I", protoDir} - args = append(args, fnames...) +func (g *GenprotoGenerator) protoc(fileNames []string) error { + args := []string{"--experimental_allow_proto3_optional", fmt.Sprintf("--go_out=plugins=grpc:%s/generated", g.genprotoDir), "-I", g.googleapisDir, "-I", g.protoSrcDir} + args = append(args, fileNames...) c := command("protoc", args...) - c.Stdout = os.Stdout - c.Stderr = os.Stderr - c.Dir = genprotoDir + c.Dir = g.genprotoDir return c.Run() } + +// getUpdatedPackages parses all of the new commits to find what packages need +// to be regenerated. +func (g *GenprotoGenerator) getUpdatedPackages(googleapisHash string) (map[string][]string, error) { + files, err := UpdateFilesSinceHash(g.googleapisDir, googleapisHash) + if err != nil { + + } + pkgFiles := make(map[string][]string) + for _, v := range files { + if !strings.HasSuffix(v, ".proto") { + continue + } + path := filepath.Join(g.googleapisDir, v) + pkg, err := goPkg(path) + if err != nil { + return nil, err + } + pkgFiles[pkg] = append(pkgFiles[pkg], path) + } + return pkgFiles, nil +} + +// moveAndCleanupGeneratedSrc moves all generated src to their correct locations +// in the repository, because protoc puts it in a folder called `generated/``. +func (g *GenprotoGenerator) moveAndCleanupGeneratedSrc() error { + log.Println("moving generated code") + // The period at the end is analogous to * (copy everything in this dir). + c := command("cp", "-R", filepath.Join(g.genprotoDir, "generated", "google.golang.org", "genproto", "googleapis"), g.genprotoDir) + if err := c.Run(); err != nil { + return err + } + + c = command("rm", "-rf", "generated") + c.Dir = g.genprotoDir + if err := c.Run(); err != nil { + return err + } + + return nil +} diff --git a/internal/gapicgen/generator/git.go b/internal/gapicgen/generator/git.go new file mode 100644 index 00000000000..8d67f3e0d9a --- /dev/null +++ b/internal/gapicgen/generator/git.go @@ -0,0 +1,138 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package generator + +import ( + "bytes" + "fmt" + "strings" +) + +// ChangeInfo represents a change and its associated metadata. +type ChangeInfo struct { + Body string + GoogleapisHash string + HasGapicChanges bool +} + +// ParseChangeInfo gets the ChangeInfo for a given googleapis hash. +func ParseChangeInfo(googleapisDir, hash string) (*ChangeInfo, error) { + // Get commit title and body + rawBody := bytes.NewBuffer(nil) + c := command("git", "show", "--pretty=format:%B", "-s", hash) + c.Stdout = rawBody + c.Dir = googleapisDir + if err := c.Run(); err != nil { + return nil, err + } + + // Add link so corresponding googleapis commit. + body := fmt.Sprintf("%s\nSource-Link: https://github.com/googleapis/googleapis/commit/%s", strings.TrimSpace(rawBody.String()), hash) + + // Try to map files updated to a package in google-cloud-go. Assumes only + // one servies protos are updated per commit. Multile versions are okay. + files, err := filesChanged(googleapisDir, hash) + if err != nil { + return nil, err + } + var pkg string + for _, file := range files { + ss := strings.Split(file, "/") + if len(ss) == 0 { + continue + } + // remove filename from path + strings.Join(ss[:len(ss)-1], "/") + tempPkg := gapicPkgs[strings.Join(ss[:len(ss)-1], "/")] + if tempPkg != "" { + pkg = tempPkg + break + } + } + if pkg == "" { + return &ChangeInfo{ + Body: body, + GoogleapisHash: hash, + }, nil + } + + // Try to add in pkg affected into conventional commit scope. + bodyParts := strings.SplitN(body, "\n", 2) + if len(bodyParts) > 0 { + titleParts := strings.SplitN(bodyParts[0], ":", 2) + if len(titleParts) == 2 { + // If a scope is already provided, remove it. + if i := strings.Index(titleParts[0], "("); i > 0 { + titleParts[0] = titleParts[0][:i] + } + titleParts[0] = fmt.Sprintf("%s(%s)", titleParts[0], pkg) + bodyParts[0] = strings.Join(titleParts, ":") + } + body = strings.Join(bodyParts, "\n") + } + + return &ChangeInfo{ + Body: body, + GoogleapisHash: hash, + HasGapicChanges: true, + }, nil +} + +// CommitsSinceHash gathers all of the commits since the provided hash for the +// given gitDir. The inclusive parameter tells if the provided hash should also +// be returned in the slice. +func CommitsSinceHash(gitDir, hash string, inclusive bool) ([]string, error) { + var commitRange string + if inclusive { + commitRange = fmt.Sprintf("%s^..", hash) + } else { + commitRange = fmt.Sprintf("%s..", hash) + } + + out := bytes.NewBuffer(nil) + c := command("git", "rev-list", commitRange) + c.Stdout = out + c.Dir = gitDir + if err := c.Run(); err != nil { + return nil, err + } + return strings.Split(strings.TrimSpace(out.String()), "\n"), nil +} + +// UpdateFilesSinceHash returns a listed of files updated since the provided +// hash for the given gitDir. +func UpdateFilesSinceHash(gitDir, hash string) ([]string, error) { + out := bytes.NewBuffer(nil) + c := command("git", "diff-tree", "--no-commit-id", "--name-only", "-r", fmt.Sprintf("%s..HEAD", hash)) + c.Stdout = out + c.Dir = gitDir + if err := c.Run(); err != nil { + return nil, err + } + return strings.Split(out.String(), "\n"), nil +} + +// filesChanged returns a list of files changed in a commit for the provdied +// hash in the given gitDir. +func filesChanged(gitDir, hash string) ([]string, error) { + out := bytes.NewBuffer(nil) + c := command("git", "show", "--pretty=format:", "--name-only", hash) + c.Stdout = out + c.Dir = gitDir + if err := c.Run(); err != nil { + return nil, err + } + return strings.Split(out.String(), "\n"), nil +} From 5615d6a550c794954b758e9a3848f9f8d6ccddc4 Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Tue, 10 Nov 2020 08:48:25 -0700 Subject: [PATCH 2/3] code review comments --- internal/gapicgen/cmd/genbot/generate.go | 2 +- internal/gapicgen/cmd/genbot/github.go | 10 +- internal/gapicgen/generator/config.go | 20 ---- internal/gapicgen/generator/generator.go | 12 +-- internal/gapicgen/generator/git.go | 119 +++++++++++++---------- internal/gapicgen/generator/git_test.go | 36 +++++++ 6 files changed, 115 insertions(+), 84 deletions(-) create mode 100644 internal/gapicgen/generator/git_test.go diff --git a/internal/gapicgen/cmd/genbot/generate.go b/internal/gapicgen/cmd/genbot/generate.go index d53740ffbd5..aa0725003e7 100644 --- a/internal/gapicgen/cmd/genbot/generate.go +++ b/internal/gapicgen/cmd/genbot/generate.go @@ -146,7 +146,7 @@ func gitClone(repo, dir string) error { func hasChanges(dir string) (bool, error) { // Write command output to both os.Stderr and local, so that we can check // whether there are modified files. - inmem := bytes.NewBuffer(nil) + inmem := &bytes.Buffer{} w := io.MultiWriter(os.Stderr, inmem) c := exec.Command("bash", "-c", "git status --short") diff --git a/internal/gapicgen/cmd/genbot/github.go b/internal/gapicgen/cmd/genbot/github.go index fb7a9d3e3ee..4ae26f62fc3 100644 --- a/internal/gapicgen/cmd/genbot/github.go +++ b/internal/gapicgen/cmd/genbot/github.go @@ -181,13 +181,13 @@ func (gc *GithubClient) GetRegenPR(ctx context.Context, repo string, status stri // hasCorrespondingPR indicates that there is a corresponding google-cloud-go PR. func (gc *GithubClient) CreateGenprotoPR(ctx context.Context, genprotoDir string, hasCorrespondingPR bool, changes []*generator.ChangeInfo) (prNumber int, _ error) { log.Println("creating genproto PR") - var body strings.Builder - body.WriteString(genprotoCommitBody) + var bodyBuilder strings.Builder + bodyBuilder.WriteString(genprotoCommitBody) if !hasCorrespondingPR { - body.WriteString("\n\nThere is no corresponding google-cloud-go PR.\n") - body.WriteString(formatChanges(changes, false)) + bodyBuilder.WriteString("\n\nThere is no corresponding google-cloud-go PR.\n") + bodyBuilder.WriteString(formatChanges(changes, false)) } - sBody := body.String() + sBody := bodyBuilder.String() c := exec.Command("/bin/bash", "-c", ` set -ex diff --git a/internal/gapicgen/generator/config.go b/internal/gapicgen/generator/config.go index f6149dfb979..a4871621eba 100644 --- a/internal/gapicgen/generator/config.go +++ b/internal/gapicgen/generator/config.go @@ -14,26 +14,6 @@ package generator -import "strings" - -// gapicPkgs is a map of googleapis inputDirectoryPath to the gapic package name -// used for conventional commits. -var gapicPkgs map[string]string - -func init() { - gapicPkgs = make(map[string]string) - for _, v := range microgenGapicConfigs { - gapicPkgs[v.inputDirectoryPath] = parseConventionalCommitPkg(v.importPath) - } -} - -func parseConventionalCommitPkg(importPath string) string { - s := strings.TrimPrefix(importPath, "cloud.google.com/go/") - ss := strings.Split(s, "/") - // remove the version, i.e /apiv1 - return strings.Join(ss[:len(ss)-1], "/") -} - // microgenConfig represents a single microgen target. type microgenConfig struct { // inputDirectoryPath is the path to the input (.proto, etc) files, relative diff --git a/internal/gapicgen/generator/generator.go b/internal/gapicgen/generator/generator.go index 14d2accc418..407c5cf5302 100644 --- a/internal/gapicgen/generator/generator.go +++ b/internal/gapicgen/generator/generator.go @@ -59,13 +59,9 @@ func gatherChanges(googleapisDir, genprotoDir string) ([]*ChangeInfo, error) { if err != nil { return nil, err } - var changes []*ChangeInfo - for _, v := range commits { - c, err := ParseChangeInfo(googleapisDir, v) - if err != nil { - return nil, err - } - changes = append(changes, c) + changes, err := ParseChangeInfo(googleapisDir, commits) + if err != nil { + return nil, err } return changes, nil @@ -84,7 +80,7 @@ func recordGoogleapisHash(googleapisDir, genprotoDir string) error { f, err := os.OpenFile(filepath.Join(genprotoDir, "regen.txt"), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644) if err != nil { - log.Fatal(err) + return err } defer f.Close() if _, err := f.WriteString(commits[0]); err != nil { diff --git a/internal/gapicgen/generator/git.go b/internal/gapicgen/generator/git.go index 8d67f3e0d9a..0e7faa4ed88 100644 --- a/internal/gapicgen/generator/git.go +++ b/internal/gapicgen/generator/git.go @@ -28,66 +28,85 @@ type ChangeInfo struct { } // ParseChangeInfo gets the ChangeInfo for a given googleapis hash. -func ParseChangeInfo(googleapisDir, hash string) (*ChangeInfo, error) { - // Get commit title and body - rawBody := bytes.NewBuffer(nil) - c := command("git", "show", "--pretty=format:%B", "-s", hash) - c.Stdout = rawBody - c.Dir = googleapisDir - if err := c.Run(); err != nil { - return nil, err +func ParseChangeInfo(googleapisDir string, hashes []string) ([]*ChangeInfo, error) { + var changes []*ChangeInfo + // gapicPkgs is a map of googleapis inputDirectoryPath to the gapic package + // name used for conventional commits. + gapicPkgs := make(map[string]string) + for _, v := range microgenGapicConfigs { + gapicPkgs[v.inputDirectoryPath] = parseConventionalCommitPkg(v.importPath) } - // Add link so corresponding googleapis commit. - body := fmt.Sprintf("%s\nSource-Link: https://github.com/googleapis/googleapis/commit/%s", strings.TrimSpace(rawBody.String()), hash) + for _, hash := range hashes { + // Get commit title and body + rawBody := bytes.NewBuffer(nil) + c := command("git", "show", "--pretty=format:%B", "-s", hash) + c.Stdout = rawBody + c.Dir = googleapisDir + if err := c.Run(); err != nil { + return nil, err + } - // Try to map files updated to a package in google-cloud-go. Assumes only - // one servies protos are updated per commit. Multile versions are okay. - files, err := filesChanged(googleapisDir, hash) - if err != nil { - return nil, err - } - var pkg string - for _, file := range files { - ss := strings.Split(file, "/") - if len(ss) == 0 { - continue + // Add link so corresponding googleapis commit. + body := fmt.Sprintf("%s\nSource-Link: https://github.com/googleapis/googleapis/commit/%s", strings.TrimSpace(rawBody.String()), hash) + + // Try to map files updated to a package in google-cloud-go. Assumes only + // one servies protos are updated per commit. Multile versions are okay. + files, err := filesChanged(googleapisDir, hash) + if err != nil { + return nil, err } - // remove filename from path - strings.Join(ss[:len(ss)-1], "/") - tempPkg := gapicPkgs[strings.Join(ss[:len(ss)-1], "/")] - if tempPkg != "" { - pkg = tempPkg - break + var pkg string + for _, file := range files { + ss := strings.Split(file, "/") + if len(ss) == 0 { + continue + } + // remove filename from path + strings.Join(ss[:len(ss)-1], "/") + tempPkg := gapicPkgs[strings.Join(ss[:len(ss)-1], "/")] + if tempPkg != "" { + pkg = tempPkg + break + } + } + if pkg == "" { + changes = append(changes, &ChangeInfo{ + Body: body, + GoogleapisHash: hash, + }) + continue } - } - if pkg == "" { - return &ChangeInfo{ - Body: body, - GoogleapisHash: hash, - }, nil - } - // Try to add in pkg affected into conventional commit scope. - bodyParts := strings.SplitN(body, "\n", 2) - if len(bodyParts) > 0 { - titleParts := strings.SplitN(bodyParts[0], ":", 2) - if len(titleParts) == 2 { - // If a scope is already provided, remove it. - if i := strings.Index(titleParts[0], "("); i > 0 { - titleParts[0] = titleParts[0][:i] + // Try to add in pkg affected into conventional commit scope. + bodyParts := strings.SplitN(body, "\n", 2) + if len(bodyParts) > 0 { + titleParts := strings.SplitN(bodyParts[0], ":", 2) + if len(titleParts) == 2 { + // If a scope is already provided, remove it. + if i := strings.Index(titleParts[0], "("); i > 0 { + titleParts[0] = titleParts[0][:i] + } + titleParts[0] = fmt.Sprintf("%s(%s)", titleParts[0], pkg) + bodyParts[0] = strings.Join(titleParts, ":") } - titleParts[0] = fmt.Sprintf("%s(%s)", titleParts[0], pkg) - bodyParts[0] = strings.Join(titleParts, ":") + body = strings.Join(bodyParts, "\n") } - body = strings.Join(bodyParts, "\n") + + changes = append(changes, &ChangeInfo{ + Body: body, + GoogleapisHash: hash, + HasGapicChanges: true, + }) } + return changes, nil +} - return &ChangeInfo{ - Body: body, - GoogleapisHash: hash, - HasGapicChanges: true, - }, nil +func parseConventionalCommitPkg(importPath string) string { + s := strings.TrimPrefix(importPath, "cloud.google.com/go/") + ss := strings.Split(s, "/") + // remove the version, i.e /apiv1 + return strings.Join(ss[:len(ss)-1], "/") } // CommitsSinceHash gathers all of the commits since the provided hash for the diff --git a/internal/gapicgen/generator/git_test.go b/internal/gapicgen/generator/git_test.go new file mode 100644 index 00000000000..b971259a4be --- /dev/null +++ b/internal/gapicgen/generator/git_test.go @@ -0,0 +1,36 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package generator + +import "testing" + +func TestParseConventionalCommitPkg(t *testing.T) { + tests := []struct { + name string + importPath string + want string + }{ + {name: "one path element", importPath: "cloud.google.com/go/foo/apiv1", want: "foo"}, + {name: "two path elements", importPath: "cloud.google.com/go/foo/bar/apiv1", want: "foo/bar"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := parseConventionalCommitPkg(tc.importPath); got != tc.want { + t.Errorf("parseConventionalCommitPkg(%q) = %q, want %q", tc.importPath, got, tc.want) + } + }) + } +} From 86667700e9c707f13f47f8b30707b2dc03a611db Mon Sep 17 00:00:00 2001 From: Cody Oss Date: Tue, 10 Nov 2020 12:33:35 -0700 Subject: [PATCH 3/3] codereview comments --- internal/gapicgen/cmd/genbot/github.go | 30 +++++++++++++------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/internal/gapicgen/cmd/genbot/github.go b/internal/gapicgen/cmd/genbot/github.go index 4ae26f62fc3..b8c2141bc88 100644 --- a/internal/gapicgen/cmd/genbot/github.go +++ b/internal/gapicgen/cmd/genbot/github.go @@ -181,13 +181,13 @@ func (gc *GithubClient) GetRegenPR(ctx context.Context, repo string, status stri // hasCorrespondingPR indicates that there is a corresponding google-cloud-go PR. func (gc *GithubClient) CreateGenprotoPR(ctx context.Context, genprotoDir string, hasCorrespondingPR bool, changes []*generator.ChangeInfo) (prNumber int, _ error) { log.Println("creating genproto PR") - var bodyBuilder strings.Builder - bodyBuilder.WriteString(genprotoCommitBody) + var sb strings.Builder + sb.WriteString(genprotoCommitBody) if !hasCorrespondingPR { - bodyBuilder.WriteString("\n\nThere is no corresponding google-cloud-go PR.\n") - bodyBuilder.WriteString(formatChanges(changes, false)) + sb.WriteString("\n\nThere is no corresponding google-cloud-go PR.\n") + sb.WriteString(formatChanges(changes, false)) } - sBody := bodyBuilder.String() + body := sb.String() c := exec.Command("/bin/bash", "-c", ` set -ex @@ -209,7 +209,7 @@ git push origin $BRANCH_NAME fmt.Sprintf("PATH=%s", os.Getenv("PATH")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. fmt.Sprintf("HOME=%s", os.Getenv("HOME")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. fmt.Sprintf("COMMIT_TITLE=%s", genprotoCommitTitle), - fmt.Sprintf("COMMIT_BODY=%s", sBody), + fmt.Sprintf("COMMIT_BODY=%s", body), fmt.Sprintf("BRANCH_NAME=%s", genprotoBranchName), } c.Dir = genprotoDir @@ -222,7 +222,7 @@ git push origin $BRANCH_NAME t := genprotoCommitTitle // Because we have to take the address. pr, _, err := gc.cV3.PullRequests.Create(ctx, "googleapis", "go-genproto", &github.NewPullRequest{ Title: &t, - Body: &sBody, + Body: &body, Head: &head, Base: &base, Draft: github.Bool(true), @@ -254,17 +254,17 @@ git push origin $BRANCH_NAME func (gc *GithubClient) CreateGocloudPR(ctx context.Context, gocloudDir string, genprotoPRNum int, changes []*generator.ChangeInfo) (prNumber int, _ error) { log.Println("creating google-cloud-go PR") - body := &strings.Builder{} + var sb strings.Builder var draft bool - body.WriteString(gocloudCommitBody) + sb.WriteString(gocloudCommitBody) if genprotoPRNum > 0 { - body.WriteString(fmt.Sprintf("\n\nCorresponding genproto PR: https://github.com/googleapis/go-genproto/pull/%d\n", genprotoPRNum)) + sb.WriteString(fmt.Sprintf("\n\nCorresponding genproto PR: https://github.com/googleapis/go-genproto/pull/%d\n", genprotoPRNum)) draft = true } else { - body.WriteString("\n\nThere is no corresponding genproto PR.\n") + sb.WriteString("\n\nThere is no corresponding genproto PR.\n") } - body.WriteString(formatChanges(changes, true)) - sBody := body.String() + sb.WriteString(formatChanges(changes, true)) + body := sb.String() c := exec.Command("/bin/bash", "-c", ` set -ex @@ -286,7 +286,7 @@ git push origin $BRANCH_NAME fmt.Sprintf("PATH=%s", os.Getenv("PATH")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. fmt.Sprintf("HOME=%s", os.Getenv("HOME")), // TODO(deklerk): Why do we need to do this? Doesn't seem to be necessary in other exec.Commands. fmt.Sprintf("COMMIT_TITLE=%s", gocloudCommitTitle), - fmt.Sprintf("COMMIT_BODY=%s", sBody), + fmt.Sprintf("COMMIT_BODY=%s", body), fmt.Sprintf("BRANCH_NAME=%s", gocloudBranchName), } c.Dir = gocloudDir @@ -297,7 +297,7 @@ git push origin $BRANCH_NAME t := gocloudCommitTitle // Because we have to take the address. pr, _, err := gc.cV3.PullRequests.Create(ctx, "googleapis", "google-cloud-go", &github.NewPullRequest{ Title: &t, - Body: &sBody, + Body: &body, Head: github.String(fmt.Sprintf("googleapis:" + gocloudBranchName)), Base: github.String("master"), Draft: github.Bool(draft),