Add --config-data flag #224

bufdev · 2018-09-07T11:40:12Z

This adds the --config-data flag.

Having played with this, I actually do wish we added a --config-dir or --config-dir-path flag as well, If you play with it locally you might see what I mean, but I understand the reasoning not to add it.

A nice representation of having a JSON file and then cat'ing it on the command line would be to cd into internal/cmd/testdata/format/proto3 and then doing:

$ prototool lint
foo/foo.proto:54:3:C-Style comments are not allowed.
foo/foo.proto:112:3:Enum aliases are not allowed.

$ prototool lint --config-data "$(cat prototool.json)"
foo/foo.proto:54:3:C-Style comments are not allowed.
foo/foo.proto:112:3:Enum aliases are not allowed.

Note if you have bash completion set up on your computer and installed with brew install prototool, you need to do brew uninstall prototool for prototool to be picked up on your $PATH from make install, as bash completion will still make it so that the Homebrew prototool binary is used, regardless of the order of the directories in your $PATH.

…son.Unmarshal if the file is empty

codecov-io · 2018-09-07T12:20:01Z

Codecov Report

Merging #224 into dev will increase coverage by 0.53%.
The diff coverage is 84%.

@@            Coverage Diff             @@
##              dev     #224      +/-   ##
==========================================
+ Coverage   69.11%   69.65%   +0.53%     
==========================================
  Files          73       73              
  Lines        3785     3865      +80     
==========================================
+ Hits         2616     2692      +76     
- Misses        844      846       +2     
- Partials      325      327       +2

Impacted Files	Coverage Δ
internal/settings/settings.go	`66.66% <ø> (ø)`	⬆️
internal/exec/exec.go	`72.72% <100%> (+4.3%)`	⬆️
internal/file/file.go	`85.71% <100%> (+2.38%)`	⬆️
internal/cmd/flags.go	`100% <100%> (ø)`	⬆️
internal/cmd/templates.go	`83.25% <100%> (+3.25%)`	⬆️
internal/settings/config_provider.go	`72.24% <62.5%> (-0.81%)`	⬇️
internal/file/proto_set_provider.go	`68.78% <76.47%> (+2.64%)`	⬆️
internal/exec/runner.go	`51.17% <77.77%> (+0.99%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8aa3306...a888d49. Read the comment docs.

amckinney

Looks good overall! Let's see if we can remove the configDirPath field and simplify this a little.

amckinney · 2018-09-07T20:15:33Z

CHANGELOG.md

@@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ## [Unreleased]
 - Accept `prototool.json` files for configuation in addition to
  `prototool.yaml` files.
+- Add `--config-data` flag.


Let's add more context here, such as how this can be used and/or is useful.

amckinney · 2018-09-07T20:17:31Z

internal/cmd/flags.go

@@ -63,6 +64,10 @@ func (f *flags) bindCallTimeout(flagSet *pflag.FlagSet) {
 	flagSet.StringVar(&f.callTimeout, "call-timeout", "60s", "The maximum time to for all calls to be completed.")
 }

+func (f *flags) bindConfigData(flagSet *pflag.FlagSet) {
+	flagSet.StringVar(&f.configData, "config-data", "", "The configuration data to use instead of reading prototool.yaml or prototool.json files.\nThis will act as if there is a configuration file with the given data in the current directory, and no other configuration files recursively.\nThis is an advanced feature and is not recommended to be generally used.")


I think we should shorten this description. The first sentence will likely suffice:

The configuration data to use instead of reading prototool.yaml or prototool.json files.

Hmmm, I disagree, let's cover this today offline?

amckinney · 2018-09-07T20:20:05Z

internal/cmd/templates.go

@@ -45,6 +45,7 @@ var (
 			return runner.All(args, flags.disableFormat, flags.disableLint, flags.fix)
 		},
 		BindFlags: func(flagSet *pflag.FlagSet, flags *flags) {
+			flags.bindConfigData(flagSet)


Why not just make this a global flag, similar to debug? I realize that some commands might not need a configuration, such as version or config init, but the same could be said about debug, so adding this to the global set seems fine to me. What are your thoughts?

I'm indifferent here, since version/files/config won't rely on it. It would be nice as a global flag for discovery purposes.

This isn't global, by the same argument, --protoc-url should be global and we chose that not to be global.

amckinney · 2018-09-07T20:27:54Z

internal/file/proto_set_provider.go

@@ -33,6 +33,8 @@ import (

 type protoSetProvider struct {
 	logger         *zap.Logger
+	configDirPath  string


We should be able to drop this field (see below).

amckinney · 2018-09-07T20:33:31Z

internal/file/proto_set_provider.go

-	configDirPath := absDirPath
-	if configFilePath != "" {
-		configDirPath = filepath.Dir(configFilePath)
+	configDirPath := c.configDirPath


Let's remove protoSetProvider.configDirPath and just use the workDirPath instead. They should always be the same.

configDirPath := absDirPath if c.configData { // The users has specified configuration via the command line. Set the configuration // directory to the current working directory. configDirPath = workDirPath } else { configFilePath, err := c.configProvider.GetFilePathForDir(absDirPath) if err != nil { return nil, err } // We need everything for generation, not just the files in the given directory. // Traverse back to the config file if it is shallower. // The display path will be unaffected as this is based on workDirPath. if configFilePath != "" { configDirPath = filepath.Dir(configFilePath) } } ...

This will let us drop the validateOptions helper, too.

This logic is slightly wrong, you never use absDirPath as a value for configDirPath directly and the else is therefore not needed, but corrected it and updated.

Also note that this now requires propagating the work dir path to one more place, but we were already propagating it.

amckinney · 2018-09-07T20:36:49Z

internal/file/proto_set_provider.go

@@ -195,6 +218,17 @@ func (c *protoSetProvider) walkAndGetAllProtoFiles(absWorkDirPath string, absDir
 		timedOut       bool
 	)
 	allExcludes := make(map[string]struct{})
+	// if we have a configDirPath and configData, we compute the exclude prefixes once
+	// from this dirPath and data, and do not do it again in the below walk function
+	if c.configDirPath != "" {


We can update this to c.configData != ""

amckinney · 2018-09-07T20:37:38Z

internal/file/proto_set_provider.go

-					for _, exclude := range excludes {
-						allExcludes[exclude] = struct{}{}
+					// Do not add if we have configDirPath and configData.
+					if c.configDirPath == "" {


We can update this to c.configData == ""

amckinney · 2018-09-07T20:38:37Z

internal/file/proto_set_provider.go

+		return fmt.Errorf("both configDirPath and configData in protoSetProvider must be set or unset")
+	}
+	return nil
+}


This shouldn't be necessary (commented above).

amckinney · 2018-09-07T20:39:02Z

internal/settings/config_provider.go

+	}
+	dirPath = filepath.Clean(dirPath)
+	return externalConfigToConfig(externalConfig, dirPath)
+}


amckinney · 2018-09-07T20:39:52Z

internal/settings/config_provider.go

@@ -83,6 +92,18 @@ func (c *configProvider) GetExcludePrefixesForDir(dirPath string) ([]string, err
 	return getExcludePrefixesForDir(dirPath)
 }

+func (c *configProvider) GetExcludePrefixesForData(dirPath string, externalConfigData string) ([]string, error) {
+	if !filepath.IsAbs(dirPath) {


Should we have this validation in GetForData, too?

peats-bond

Before this change, the protoSetProvider required no information about configuration and I think we should keep it that way. We're leaking configuration information into protoSetProvider when instead this should be contained within the configProvider -- a protoSetProvider shouldn't care how configuration got there, just that it exists.

A protoSetProvider already holds a configProvider, which means that the protoSetProviders current implementation can be left virtually unchanged. Instead of creating a ProtoSetProviderWithConfigData option, we should replace it with an equivalent ConfigProviderOption. This would enable us to leave the ConfigProvider interface unchanged too so we'd remove a lot of the duplication that's been introduced. I think we can get a lot of simplification by going this alternate route.

Some of the comments @amckinney's left are still relevant with this suggested change. Specifically, not needing the configDirPath.

peats-bond · 2018-09-10T22:03:24Z

internal/cmd/templates.go

@@ -45,6 +45,7 @@ var (
 			return runner.All(args, flags.disableFormat, flags.disableLint, flags.fix)
 		},
 		BindFlags: func(flagSet *pflag.FlagSet, flags *flags) {
+			flags.bindConfigData(flagSet)


I'm indifferent here, since version/files/config won't rely on it. It would be nice as a global flag for discovery purposes.

peats-bond · 2018-09-10T22:22:15Z

internal/file/proto_set_provider.go

-	configDirPath := absDirPath
-	if configFilePath != "" {
-		configDirPath = filepath.Dir(configFilePath)
+	configDirPath := c.configDirPath


bufdev · 2018-09-11T14:59:29Z

@AlexAPB perhaps we should sync offline - I don't get what you're concretely suggesting. The same amount of information is leaked from what I see, and changing the logic is a significant refactor. Maybe I'm missing something, but we should sync offline and/or I would want a concrete suggestion on how to proceed.

peats-bond

Talked offline. My reservations with this change would be rectified by a much larger refactor that we don't really have the bandwidth for right now. This is fine for now, since are our 1.0 commitment is to the public commands and flags.

For a clean up change (sometime in the future) we should ideally create the settings provider once and thread that information along throughout the invocation. It would make changes like this less leaky.

bufdev added 9 commits September 7, 2018 12:01

Allow prototool.json

77c457d

Testing

763f856

Documentation

1492714

Change some integration tests to use prototool.yaml and do not call j…

6c27360

…son.Unmarshal if the file is empty

Add --config-data flag

eac22ec

Add testing

d1a136b

add jsonUnmarshalStrict

cbc6749

Merge branch 'prototool-json' into config-data-flag

2373d93

Switch to jsonUnmarshalStrict and add test for unknown JSON key

bbd8955

bufdev changed the title ~~WIP: add --config-data flag~~ Add --config-data flag Sep 7, 2018

bufdev requested review from peats-bond and amckinney September 7, 2018 12:31

bufdev mentioned this pull request Sep 7, 2018

Install command to explicitly trigger a protoc install #202

Closed

bufdev added 3 commits September 7, 2018 19:02

Merge branch 'dev' into prototool-json

63cbb7c

Address PR comment

aa0561f

Merge branch 'prototool-json' into config-data-flag

4d111fc

amckinney reviewed Sep 7, 2018

View reviewed changes

peats-bond suggested changes Sep 10, 2018

View reviewed changes

peats-bond mentioned this pull request Sep 10, 2018

Override values in prototool.yaml #105

Closed

Address PR comment

78737d4

bufdev added 3 commits September 12, 2018 13:43

Merge branch 'dev' into prototool-json

590b977

Address PR comment

1577e83

Merge branch 'prototool-json' into config-data-flag

2161cca

bufdev changed the base branch from prototool-json to dev September 12, 2018 11:51

bufdev added 2 commits September 12, 2018 13:55

Merge

2d53437

Address PR comments

a888d49

peats-bond approved these changes Sep 14, 2018

View reviewed changes

amckinney merged commit 3f348ae into dev Sep 14, 2018

amckinney deleted the config-data-flag branch September 14, 2018 23:03

sakshamsaxena mentioned this pull request Feb 25, 2021

--config-data is partial to accepting JSON config data only, and not YAML #575

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --config-data flag #224

Add --config-data flag #224

bufdev commented Sep 7, 2018 •

edited

Loading

codecov-io commented Sep 7, 2018 •

edited

Loading

amckinney left a comment

amckinney Sep 7, 2018

amckinney Sep 7, 2018

bufdev Sep 12, 2018

amckinney Sep 7, 2018

peats-bond Sep 10, 2018

bufdev Sep 11, 2018

amckinney Sep 7, 2018

amckinney Sep 7, 2018

peats-bond Sep 10, 2018

bufdev Sep 12, 2018

bufdev Sep 12, 2018

amckinney Sep 7, 2018

bufdev Sep 12, 2018

amckinney Sep 7, 2018

bufdev Sep 12, 2018

amckinney Sep 7, 2018

amckinney Sep 7, 2018

amckinney Sep 7, 2018

bufdev Sep 12, 2018

peats-bond left a comment

peats-bond Sep 10, 2018

peats-bond Sep 10, 2018

bufdev commented Sep 11, 2018

peats-bond left a comment •

edited

Loading

Add --config-data flag #224

Add --config-data flag #224

Conversation

bufdev commented Sep 7, 2018 • edited Loading

codecov-io commented Sep 7, 2018 • edited Loading

Codecov Report

amckinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peats-bond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bufdev commented Sep 11, 2018

peats-bond left a comment • edited Loading

Choose a reason for hiding this comment

bufdev commented Sep 7, 2018 •

edited

Loading

codecov-io commented Sep 7, 2018 •

edited

Loading

peats-bond left a comment •

edited

Loading