Repository: uber/prototool
Pull request #382 (2019-03-07 16:52:16)

Allow the breaking change detector to check against a FileDescriptorSet file
This PR does a few things:

- Moves the `descriptor-set` command out of `x`. In #381, I said there were unanswered questions about whether to print to stdout, but having used this more this morning, this feature is too valuable to leave in `x`. Being able to do things like `prototool descriptor-set path/to/proto/root --json | jq '.file[] | select(.messageType != null) | .messageType[] | .name' | sort | uniq` is very nice.
- Adds a `--descriptor-set-path` flag (aliased as `-f`) to `break check` that allows the breaking change detector to check against a file containing a serialized `FileDescriptorSet` (analogous to `proto.lock` in github.com/nilslice/protolock). This removes any `git` operations from `break check` if specified. This allows someone to generate a `FileDescriptorSet` file and check it in, instead of relying on `git`. Not only is this faster, it's useful when a `git` repository is not present.
- Adds a `break descriptor-set` sub-command that generates serialized `FileDescriptorSet` files. This is an alias for `descriptor-set --include-imports`, where `-o` is required.

The result is the ability to do the following workflow:

```
# on check-in
prototool break descriptor-set path/to/proto/root -o descriptor.bin
# when you want to check if there are breaking changes at your current state
prototool break check path/to/proto/root -f descriptor.bin
```

This is in addition to the existing default workflow:

```
# clone the default branch, compile the definitions in path/to/proto/root at that point, compare
prototool break check path/to/proto/root
# alternatively, check against a non-default branch or tag
prototool break check path/to/proto/root --git-branch v1.0.0
```

The result is a compelling tool that covers more use cases:

- One can check against a git branch or tag, cloning and compiling on the fly.
- One can check against a `FileDescriptorSet`.

The other benefit of this breaking change detector, namely that as opposed to other tools, `break check` checks on a per-package basis instead of a per-file basis (so that one can move definitions between files as long as they are in the same Protobuf package, as should be allowed), is still present.

I've manually tested this works as I didn't have integration testing for `break check` previously, but the workflow is tested up to the actual point of the CLI.

codecov-io: # [Codecov](https://codecov.io/gh/uber/prototool/pull/382?src=pr&el=h1) Report
> Merging [#382](https://codecov.io/gh/uber/prototool/pull/382?src=pr&el=desc) into [dev](https://codecov.io/gh/uber/prototool/commit/08de734fd8e956b6eefdfda4ec1baa179ab5da93?src=pr&el=desc) will **decrease** coverage by `0.18%`.
> The diff coverage is `29.16%`.

[![Impacted file tree graph](https://codecov.io/gh/uber/prototool/pull/382/graphs/tree.svg?width=650&token=A9V0DApqZg&height=150&src=pr)](https://codecov.io/gh/uber/prototool/pull/382?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##              dev     #382      +/-   ##
==========================================
- Coverage   73.19%   73.01%   -0.19%     
==========================================
  Files         143      143              
  Lines        6623     6664      +41     
==========================================
+ Hits         4848     4866      +18     
- Misses       1309     1332      +23     
  Partials      466      466
```


| [Impacted Files](https://codecov.io/gh/uber/prototool/pull/382?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/exec/exec.go](https://codecov.io/gh/uber/prototool/pull/382/diff?src=pr&el=tree#diff-aW50ZXJuYWwvZXhlYy9leGVjLmdv) | `61.29% <ø> (ø)` | :arrow_up: |
| [internal/cmd/flags.go](https://codecov.io/gh/uber/prototool/pull/382/diff?src=pr&el=tree#diff-aW50ZXJuYWwvY21kL2ZsYWdzLmdv) | `100% <100%> (ø)` | :arrow_up: |
| [internal/cmd/cmd.go](https://codecov.io/gh/uber/prototool/pull/382/diff?src=pr&el=tree#diff-aW50ZXJuYWwvY21kL2NtZC5nbw==) | `69.01% <100%> (+0.44%)` | :arrow_up: |
| [internal/exec/runner.go](https://codecov.io/gh/uber/prototool/pull/382/diff?src=pr&el=tree#diff-aW50ZXJuYWwvZXhlYy9ydW5uZXIuZ28=) | `45.52% <7.84%> (-1.12%)` | :arrow_down: |
| [internal/cmd/templates.go](https://codecov.io/gh/uber/prototool/pull/382/diff?src=pr&el=tree#diff-aW50ZXJuYWwvY21kL3RlbXBsYXRlcy5nbw==) | `78.61% <73.33%> (-0.01%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/uber/prototool/pull/382?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/uber/prototool/pull/382?src=pr&el=footer). Last update [08de734...a83bd9c](https://codecov.io/gh/uber/prototool/pull/382?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6f37c17..1ea20f3 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,16 +11,27 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
   version of our lint rules. These rules are almost entirely a superset of the
   V1 Style guide lint rules. If `lint.group` is set to `uber2`, this also will
   affect the `create` and `format` commands, as the `uber2` lint group adds
-  more file options to more closely match the [Google Cloud APIs File Structure](https://cloud.google.com/apis/design/file_structure).
+  more file options to more closely match the [Google Cloud APIs File Structure](https://cloud.google.com/apis/design/file_structure)
+  and changes the value of `go_package` to take versions into account.
   In total, 39 lint rules have been added as compared to the `uber1` lint
   group.
-  and changes the value of `go_package` to take versions into account.
 - New `google` lint group representing Google's minimal [Style Guide](https://developers.google.com/protocol-buffers/docs/style).
 - Add `--list-lint-group` flag to the `lint` command to list a lint group's
   rules.
 - Add `--diff-lint-groups` flag to the `lint` command to print the diff
   between two lint groups.
-- Breaking change detector added as the `break check` command.
+- Add `descriptor-set` command to output a merged `FileDescriptorSet`
+  with all files compiled to either stdout, a given file, or a temporary file.
+  Useful with external tools that use FileDescriptorSets, and also useful for
+  inspection if the `--json` flag is given.
+- Add breaking change detector as the `break check` command. By default, this
+  compiles your existing Protobuf definitions, and then does a shallow clone
+  of your git repository against the default branch and compiles the
+  definitions on that branch, and compares the existing versus the branch.
+  The branch can be controlled with the `--git-branch` flag, and one can
+  use a `FileDescriptorSet` instead of a shallow clone by generating a
+  file with `break descriptor-set` and then passing the path to this file
+  to `break check` with the `--descriptor-set-path` flag.
 - A Docker image is now provided on Docker Hub as [uber/prototool](https://hub.docker.com/r/uber/prototool)
   which provides an environment with commonly-used plugins.
 - Switch to Golang Modules for dependency management.
@@ -38,11 +49,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
   `descriptor_set` plugin.
 - Add `cache` top-level command to allow management of the `protoc` cache.
 - Add `x` top-level command for experimental functionality.
-- Add `descriptor-set` command under `x` to output a merged FileDescriptorSet
-  with all files compiled to either stdout, a given file, or a temporary file.
-  Useful with external tools that use FileDescriptorSets.
 - Add `inspect` command under `x` with Protobuf inspection capabilities.
 - Add `--error-format` flag to allow specific error fields to be printed.
+- Allow the `protoc` binary and WKT paths to be controlled by the environment
+  variables `PROTOTOOL_PROTOC_BIN_PATH` and `PROTOTOOL_PROTOC_WKT_PATH` in
+  addition to the existing `--protoc-bin-path` and `--protoc-wkt-path` flags.
+  The flags take precedence. This is especially useful for Docker images.
 - Add file locking around the `protoc` downloader to eliminate concurrency
   issues where multiple `prototool` invocations may be accessing the cache
   at the same time.
diff --git a/internal/cmd/cmd.go b/internal/cmd/cmd.go
index 6c3578f..91edc93 100644
--- a/internal/cmd/cmd.go
+++ b/internal/cmd/cmd.go
@@ -105,6 +105,7 @@ func getRootCommand(develMode bool, exitCodeAddr *int, args []string, stdin io.R
 	rootCmd.AddCommand(formatCmdTemplate.Build(develMode, exitCodeAddr, stdin, stdout, stderr, flags))
 	rootCmd.AddCommand(generateCmdTemplate.Build(develMode, exitCodeAddr, stdin, stdout, stderr, flags))
 	rootCmd.AddCommand(grpcCmdTemplate.Build(develMode, exitCodeAddr, stdin, stdout, stderr, flags))
+	rootCmd.AddCommand(descriptorSetCmdTemplate.Build(develMode, exitCodeAddr, stdin, stdout, stderr, flags))
 	configCmd := &cobra.Command{Use: "config", Short: "Interact with configuration files."}
 	configCmd.AddCommand(configInitCmdTemplate.Build(develMode, exitCodeAddr, stdin, stdout, stderr, flags))
 	rootCmd.AddCommand(configCmd)
@@ -116,9 +117,10 @@ func getRootCommand(develMode bool, exitCodeAddr *int, args []string, stdin io.R
 	rootCmd.AddCommand(cacheCmd)
 	breakCmd := &cobra.Command{Use: "break", Short: "Top-level command for breaking change commands."}
 	breakCmd.AddCommand(breakCheckCmdTemplate.Build(develMode, exitCodeAddr, stdin, stdout, stderr, flags))
+	breakCmd.AddCommand(breakDescriptorSetCmdTemplate.Build(develMode, exitCodeAddr, stdin, stdout, stderr, flags))
 	rootCmd.AddCommand(breakCmd)
+
 	experimentalCmd := &cobra.Command{Use: "x", Short: "Top-level command for experimental commands. These may change between minor versions."}
-	experimentalCmd.AddCommand(descriptorSetCmdTemplate.Build(develMode, exitCodeAddr, stdin, stdout, stderr, flags))
 	inspectCmd := &cobra.Command{Use: "inspect", Short: "Top-level command for inspection commands."}
 	inspectCmd.AddCommand(inspectPackagesCmdTemplate.Build(develMode, exitCodeAddr, stdin, stdout, stderr, flags))
 	inspectCmd.AddCommand(inspectPackageDepsCmdTemplate.Build(develMode, exitCodeAddr, stdin, stdout, stderr, flags))
diff --git a/internal/cmd/flags.go b/internal/cmd/flags.go
index caff8b9..7104e6a 100644
--- a/internal/cmd/flags.go
+++ b/internal/cmd/flags.go
@@ -32,6 +32,7 @@ type flags struct {
 	connectTimeout    string
 	data              string
 	debug             bool
+	descriptorSetPath string
 	details           bool
 	diffLintGroups    string
 	diffMode          bool
@@ -93,6 +94,10 @@ func (f *flags) bindDebug(flagSet *pflag.FlagSet) {
 	flagSet.BoolVar(&f.debug, "debug", false, "Run in debug mode, which will print out debug logging.")
 }
 
+func (f *flags) bindDescriptorSetPath(flagSet *pflag.FlagSet) {
+	flagSet.StringVarP(&f.descriptorSetPath, "descriptor-set-path", "f", "", "The path to the file containing a serialized FileDescriptorSet to check against.\nFileDescriptorSet files can be produced using the descriptor-set sub-command.\nThe default behavior is to check against a git branch or tag. This cannot be used with the --git-branch flag.")
+}
+
 func (f *flags) bindDetails(flagSet *pflag.FlagSet) {
 	flagSet.BoolVar(&f.details, "details", false, "Output headers, trailers, and status as well as the responses.")
 }
@@ -185,6 +190,10 @@ func (f *flags) bindOutputPath(flagSet *pflag.FlagSet) {
 	flagSet.StringVarP(&f.outputPath, "output-path", "o", "", "Write the FileDescriptorSet to the given file path instead of outputting to stdout.")
 }
 
+func (f *flags) bindOutputPathBreakDescriptorSet(flagSet *pflag.FlagSet) {
+	flagSet.StringVarP(&f.outputPath, "output-path", "o", "", "The file path to write the FileDescriptorSet to. This is required.")
+}
+
 func (f *flags) bindOverwrite(flagSet *pflag.FlagSet) {
 	flagSet.BoolVarP(&f.overwrite, "overwrite", "w", false, "Overwrite the existing file instead of writing the formatted file to stdout.")
 }
diff --git a/internal/cmd/templates.go b/internal/cmd/templates.go
index 0a1e70d..9715320 100644
--- a/internal/cmd/templates.go
+++ b/internal/cmd/templates.go
@@ -61,16 +61,15 @@ var (
 	breakCheckCmdTemplate = &cmdTemplate{
 		Use:   "check [dir]",
 		Short: "Check for breaking changes.",
-		Long: `This command must be run from the root of a git repository.
-
-The input directory must be relative.`,
-		Args: cobra.MaximumNArgs(1),
+		Long:  `This command must be run from the root of a git repository, and the input directory must be relative, unless --descriptor-set-path is specified.`,
+		Args:  cobra.MaximumNArgs(1),
 		Run: func(runner exec.Runner, args []string, flags *flags) error {
-			return runner.BreakCheck(args, flags.gitBranch)
+			return runner.BreakCheck(args, flags.gitBranch, flags.descriptorSetPath)
 		},
 		BindFlags: func(flagSet *pflag.FlagSet, flags *flags) {
 			flags.bindCachePath(flagSet)
 			flags.bindConfigData(flagSet)
+			flags.bindDescriptorSetPath(flagSet)
 			flags.bindGitBranch(flagSet)
 			flags.bindJSON(flagSet)
 			flags.bindProtocURL(flagSet)
@@ -79,6 +78,25 @@ The input directory must be relative.`,
 		},
 	}
 
+	breakDescriptorSetCmdTemplate = &cmdTemplate{
+		Use:   "descriptor-set [dir]",
+		Short: "Generate a FileDescriptorSet file that stores the current state of your Protobuf definitions for use with break check --descriptor-set-path. The -o flag is required.",
+		Args:  cobra.MaximumNArgs(1),
+		Run: func(runner exec.Runner, args []string, flags *flags) error {
+			return runner.BreakDescriptorSet(args, flags.outputPath)
+		},
+		BindFlags: func(flagSet *pflag.FlagSet, flags *flags) {
+			flags.bindCachePath(flagSet)
+			flags.bindConfigData(flagSet)
+			flags.bindErrorFormat(flagSet)
+			flags.bindJSON(flagSet)
+			flags.bindProtocURL(flagSet)
+			flags.bindProtocBinPath(flagSet)
+			flags.bindProtocWKTPath(flagSet)
+			flags.bindOutputPathBreakDescriptorSet(flagSet)
+		},
+	}
+
 	cacheUpdateCmdTemplate = &cmdTemplate{
 		Use:   "update [dirOrFile]",
 		Short: "Update the cache by downloading all artifacts.",
diff --git a/internal/exec/BUILD.bazel b/internal/exec/BUILD.bazel
index d9ca771..98763a7 100644
--- a/internal/exec/BUILD.bazel
+++ b/internal/exec/BUILD.bazel
@@ -27,6 +27,7 @@ go_library(
         "//internal/vars:go_default_library",
         "@com_github_golang_protobuf//jsonpb:go_default_library_gen",
         "@com_github_golang_protobuf//proto:go_default_library",
+        "@io_bazel_rules_go//proto/wkt:descriptor_go_proto",
         "@org_uber_go_multierr//:go_default_library",
         "@org_uber_go_zap//:go_default_library",
     ],
diff --git a/internal/exec/exec.go b/internal/exec/exec.go
index e5bc0de..542e046 100644
--- a/internal/exec/exec.go
+++ b/internal/exec/exec.go
@@ -60,7 +60,8 @@ type Runner interface {
 	InspectPackages(args []string) error
 	InspectPackageDeps(args []string, name string) error
 	InspectPackageImporters(args []string, name string) error
-	BreakCheck(args []string, gitBranch string) error
+	BreakCheck(args []string, gitBranch string, descriptorSetPath string) error
+	BreakDescriptorSet(args []string, outputPath string) error
 	DescriptorSet(args []string, includeImports bool, includeSourceInfo bool, outputPath string, tmp bool) error
 }
 
diff --git a/internal/exec/runner.go b/internal/exec/runner.go
index 59b7f46..c359e27 100644
--- a/internal/exec/runner.go
+++ b/internal/exec/runner.go
@@ -39,6 +39,7 @@ import (
 
 	"github.com/golang/protobuf/jsonpb"
 	"github.com/golang/protobuf/proto"
+	"github.com/golang/protobuf/protoc-gen-go/descriptor"
 	"github.com/uber/prototool/internal/breaking"
 	"github.com/uber/prototool/internal/cfginit"
 	"github.com/uber/prototool/internal/create"
@@ -589,6 +590,13 @@ func (r *runner) GRPC(args, headers []string, address, method, data, callTimeout
 	).Invoke(fileDescriptorSets.Unwrap(), address, method, reader, r.output)
 }
 
+func (r *runner) BreakDescriptorSet(args []string, outputPath string) error {
+	if outputPath == "" {
+		return newExitErrorf(255, "must set output-path")
+	}
+	return r.DescriptorSet(args, true, false, outputPath, false)
+}
+
 func (r *runner) DescriptorSet(args []string, includeImports bool, includeSourceInfo bool, outputPath string, tmp bool) (retErr error) {
 	if outputPath != "" && tmp {
 		return newExitErrorf(255, "can only set one of output-path, tmp")
@@ -671,46 +679,58 @@ func (r *runner) InspectPackageImporters(args []string, name string) error {
 	return r.printPackageNames(pkg.ImporterNameToImporter())
 }
 
-func (r *runner) BreakCheck(args []string, gitBranch string) error {
-	relDirPath := "."
-	// we check length 0 or 1 in cmd, similar to other commands
-	if len(args) == 1 {
-		relDirPath = args[0]
-	}
-	if filepath.IsAbs(relDirPath) {
-		return fmt.Errorf("input argument must be relative directory path: %s", relDirPath)
+func (r *runner) BreakCheck(args []string, gitBranch string, descriptorSetPath string) error {
+	if gitBranch != "" && descriptorSetPath != "" {
+		return newExitErrorf(255, "can only set one of git-branch, descriptor-set-path")
 	}
 
-	absDirPath, err := file.AbsClean(relDirPath)
-	if err != nil {
-		return err
-	}
-	absWorkDirPath, err := file.AbsClean(r.workDirPath)
+	toPackageSet, config, err := r.getPackageSetAndConfig(args)
 	if err != nil {
 		return err
 	}
-	if !strings.HasPrefix(absDirPath, absWorkDirPath) {
-		return fmt.Errorf("input directory must be within working directory: %s", relDirPath)
-	}
 
-	toPackageSet, config, err := r.getPackageSetAndConfigForRelDirPath(relDirPath)
-	if err != nil {
-		return err
-	}
+	var fromPackageSet *extract.PackageSet
+	if descriptorSetPath != "" {
+		fromPackageSet, err = r.getPackageSetForDescriptorSetPath(descriptorSetPath)
+		if err != nil {
+			return err
+		}
+	} else {
+		relDirPath := "."
+		// we check length 0 or 1 in cmd, similar to other commands
+		if len(args) == 1 {
+			relDirPath = args[0]
+		}
+		if filepath.IsAbs(relDirPath) {
+			return fmt.Errorf("input argument must be relative directory path: %s", relDirPath)
+		}
 
-	// this will purposefully fail if we are not at a git repository
-	cloneDirPath, err := git.TemporaryClone(r.logger, r.workDirPath, gitBranch)
-	if err != nil {
-		return err
-	}
-	defer func() {
-		r.logger.Sugar().Debugf("removing %s", cloneDirPath)
-		_ = os.RemoveAll(cloneDirPath)
-	}()
+		absDirPath, err := file.AbsClean(relDirPath)
+		if err != nil {
+			return err
+		}
+		absWorkDirPath, err := file.AbsClean(r.workDirPath)
+		if err != nil {
+			return err
+		}
+		if !strings.HasPrefix(absDirPath, absWorkDirPath) {
+			return fmt.Errorf("input directory must be within working directory: %s", relDirPath)
+		}
 
-	fromPackageSet, _, err := r.cloneForWorkDirPath(cloneDirPath).getPackageSetAndConfigForRelDirPath(relDirPath)
-	if err != nil {
-		return err
+		// this will purposefully fail if we are not at a git repository
+		cloneDirPath, err := git.TemporaryClone(r.logger, r.workDirPath, gitBranch)
+		if err != nil {
+			return err
+		}
+		defer func() {
+			r.logger.Sugar().Debugf("removing %s", cloneDirPath)
+			_ = os.RemoveAll(cloneDirPath)
+		}()
+
+		fromPackageSet, _, err = r.cloneForWorkDirPath(cloneDirPath).getPackageSetAndConfigForRelDirPath(relDirPath)
+		if err != nil {
+			return err
+		}
 	}
 
 	failures, err := r.newBreakingRunner().Run(config.Break, fromPackageSet, toPackageSet)
@@ -736,11 +756,7 @@ func (r *runner) getPackageSetAndConfig(args []string) (*extract.PackageSet, set
 	if err != nil {
 		return nil, settings.Config{}, err
 	}
-	reflectPackageSet, err := reflect.NewPackageSet(fileDescriptorSets.Unwrap()...)
-	if err != nil {
-		return nil, settings.Config{}, err
-	}
-	packageSet, err := extract.NewPackageSet(reflectPackageSet)
+	packageSet, err := r.getPackageSetForFileDescriptorSets(fileDescriptorSets.Unwrap()...)
 	if err != nil {
 		return nil, settings.Config{}, err
 	}
@@ -751,6 +767,26 @@ func (r *runner) getPackageSetAndConfig(args []string) (*extract.PackageSet, set
 	return packageSet, config, nil
 }
 
+func (r *runner) getPackageSetForDescriptorSetPath(descriptorSetPath string) (*extract.PackageSet, error) {
+	data, err := ioutil.ReadFile(descriptorSetPath)
+	if err != nil {
+		return nil, err
+	}
+	fileDescriptorSet := &descriptor.FileDescriptorSet{}
+	if err := proto.Unmarshal(data, fileDescriptorSet); err != nil {
+		return nil, err
+	}
+	return r.getPackageSetForFileDescriptorSets(fileDescriptorSet)
+}
+
+func (r *runner) getPackageSetForFileDescriptorSets(fileDescriptorSets ...*descriptor.FileDescriptorSet) (*extract.PackageSet, error) {
+	reflectPackageSet, err := reflect.NewPackageSet(fileDescriptorSets...)
+	if err != nil {
+		return nil, err
+	}
+	return extract.NewPackageSet(reflectPackageSet)
+}
+
 func (r *runner) getPackage(args []string, name string) (*extract.Package, error) {
 	if name == "" {
 		return nil, newExitErrorf(255, "must set name")


Test patch:
diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go
index 7f1c8bc..053a852 100644
--- a/internal/cmd/cmd_test.go
+++ b/internal/cmd/cmd_test.go
@@ -1412,7 +1412,7 @@ func assertGoldenFormat(t *testing.T, expectSuccess bool, fix bool, filePath str
 }
 
 func assertDescriptorSet(t *testing.T, expectSuccess bool, dirOrFile string, includeImports bool, includeSourceInfo bool, expectedNames ...string) {
-	args := []string{"x", "descriptor-set"}
+	args := []string{"descriptor-set"}
 	if includeImports {
 		args = append(args, "--include-imports")
 	}