gh config by vilmibm · Pull Request #728 · cli/cli

vilmibm · 2020-04-01T20:32:07Z

This PR implements gh config. Originally I was going to do several smaller PRs but things ended up being too conceptually coupled for me for that to make sense.

What this PR does:

Support a new config format (including migrating to it)
Support gh config get and gh config set
Support reading/writing editor and git_protocol values
Support host-level overrides with --host

What this PR doesn't do:

Actually respect editor and git_protocol settings
Lay out a nice default config with comments
Add a gh config top level command; it just adds get/set

TODO TODID

fixes #622

thefotios · 2020-04-01T23:24:21Z

Have you looked at https://github.com/spf13/viper? It has some nice integration with cobra and it also let's you marshal/unmarshal to interfaces.

vilmibm · 2020-04-06T21:10:46Z

@thefotios We did look at viper. It's impressive but a lot more than we currently need; more importantly, my understanding is that it would not preserve any comments in the config file. We'd like to have a very human-friendly, documented config with preserved comments.

mislav

Thanks for working on this!

mislav · 2020-04-07T14:11:51Z

command/config.go

+
+	switch len(args) {
+	case 0:
+		cfgInteractive(cmd)


I feel that some sort of interactive mode might be jumping the gun at this point, since we primarily want to allow people to set/change a configuration option.

agreed, i was just sketching. i actually meant to take all the command stuff out before even asking you to look at it, sorry

mislav · 2020-04-07T14:13:33Z

context/config_file.go

+			return hc, nil
+		}
+	}
+	return nil, fmt.Errorf("could not find config entry for \"%s\"", hostname)


Nit: you can easily display a quoted string value by using %q instead of "%s"

mislav · 2020-04-07T14:15:18Z

context/config_file.go

+				}
+				hostConfig := HostConfig{}
+				hostConfig.Host = v.Value
+				authsRoot := topLevelKeys[i+1].Content[j+1]


Is there some sort of guard that this array access won't panic if it's out of bounds?

nope -- i still have to add error handling / bounds checking

mislav · 2020-04-07T14:19:28Z

context/config_file.go

+				config.Hosts = append(config.Hosts, &hostConfig)
 			}
-			return &entries[0], nil
+		case "protocol":


I wonder if we could get away with simply storing/parsing any key-value pair and letting another mechanism query the values and decide what to do with them?

I feel as if this should be a lower-level config mechanism which doesn't necessarily understand nor care about the meaning of any configuration options, apart from parsing per-host config, and then have another configuration layer that wraps the storage layer and understands what protocol, ssh, editor and such means.

I hope that this doesn't feel like I'm suggesting over-engineering this; I do believe that a simple storage later that doesn't care about the semantics of individual keys would be the easiest to build and test!

Could we be more specific with what "protocol" refers to in configuration options, for example by renaming the key to something like clone_protocol or git_protocol?

The reason is that for GitHub enterprise, we might need to support GHE API access over the http: protocol (some intranet installs don't have GHE accessible under SSL and that's okay I guess since it's intranet). To support that case, there might be an api_protocol or similarly named key under host-specific configuration, and therefore it might be generally a good idea to avoid vagueness when using the world "protocol" unqualified

Let me know if this makes sense; just thinking ahead here. mislav/hub#2128 (comment)

I wonder if we could get away with simply storing/parsing any key-value pair and letting another mechanism query the values and decide what to do with them?

I was considering an approach like this:

continue store the root *yaml.Node in the Config struct. We need it to properly write the config back out in case it's changed via a gh config... invocation.

add methods to the Config struct that lazily parse a desired config key instead of trying to parse it all up front. Each method is responsible for traversing the root and finding the desired node and parsing it. the Hosts method would need to be involved, but the simple k/v settings can share a helper.

support a mock Config in tests (like we do with Context) whose various methods have predictable responses

unit test the yaml parsing on its own (like we already do)

This way as we add new supported config values we add new methods to Config.

This approach means we can progressively parse the yaml and not fail because of an unknown key or malformed key that isn't relevant to the current invocation of gh.

I'm working on spiking that now; hopefully you'll be able to look over it when you start your day.

ampinsk · 2020-04-07T20:52:00Z

Hey @vilmibm when you're ready, could you share a gif of this in action? 🙇‍♀

vilmibm · 2020-04-07T21:40:37Z

@ampinsk which part? for this PR i only plan on including reading and migrating to the new config format (nesting what we have now under a hosts: key); no new commands.

ampinsk · 2020-04-07T21:42:10Z

Oo my bad then! Carry on. 🙈

At some point would like to see the commands and the file itself but no rush!

mislav

Looks good so far! Thank you for the hard work

mislav · 2020-04-17T16:15:53Z

context/config_file.go

+	newConfigData := map[string]map[string]map[string]string{}
+	newConfigData["hosts"] = map[string]map[string]string{}


protip: you could alias some of these types so it looks more expressive. example:

type ConfigEntry map[string]string type ConfigMap map[string]ConfigEntry newConfigData := make(ConfigMap)

mislav · 2020-04-17T16:25:33Z

context/config_type.go

+	configForHost(string) (*HostConfig, error)
+	parseHosts(*yaml.Node) ([]*HostConfig, error)


It's unusual to me to see un-exported methods in an interface. But if it helps the implemenation then I see no harm! Alternatively, you could consider just having package-level functions configForHost() and parseHosts() that accept the config object as parameter, but then you'd have to have a switch-type statement and that could become messy.

it's leftover from when LegacyConfig was a thing. That is no longer a thing, so I just removed those from the interface.

- adds config get and config set commands - supports arbitrary k/v strings set at top and host level - supports writing an updated config, preserving comments - supports mostly lazy evaluation of yaml

mislav

Looks and works great for me in testing!

Documentation note: I think that we should omit documenting --host until we release GHE support that would take advantage of it. More thoughts in comments.

Edge case note: I edited my config file and removed the user auth setting (leaving in the token) and ran gh issue status. Instead of erroring out with the message that user could not be read, it proceeded to query the API with the username being set to an empty string (which failed on the server).

mislav · 2020-04-20T13:35:46Z

command/config.go

+var configCmd = &cobra.Command{
+	Use:   "config",
+	Short: "Set and get gh settings",
+	Long: `Get and set key/value strings. They can be optionally scoped to a particular host.


Since we don't document --host in this help text but in subcommands, I would suggest that we remove the “They can be optionally scoped” sentence and let people discover that option in get/set subcommand docs. Otherwise we give them a small hint of a feature but we don't give them any more information to understand how the feature is used.

mislav · 2020-04-20T13:40:51Z

command/config.go

+}
+
+var configGetCmd = &cobra.Command{
+	Use:   "get",


We could expand the usage synopsis to be get <key> so the parameters are easier to understand at a glance

mislav · 2020-04-20T13:57:40Z

command/config.go

+Examples:
+  gh config get git_protocol
+  https
+  gh config get --host example.com


Since we plan to ship this in the next release, but don't plan GHE support in the same release, I wonder if it makes sense to hide all reference to --host in the docs. We could mark the flag as "hidden".

The rationale would be that it makes no sense to advertise a feature that currently has no use-case. Scoping by host only makes sense once we add multi-host support.

makes total sense. that occurred to me in the last hour of getting this PR ready. I was so fixated on making it working I forgot that it wouldn't even be supported in the first release of gh config 🤦‍♀️

mislav · 2020-04-20T13:58:09Z

command/config.go

+}
+
+var configSetCmd = &cobra.Command{
+	Use:   "set",


We could expand the usage synopsis to be set <key> <value>

mislav · 2020-04-20T14:04:02Z

context/blank_context.go

+func (c *blankContext) Config() (Config, error) {
+	config, err := ParseConfig("boom.txt")
+	if err != nil {
+		panic(fmt.Sprintf("failed to parse config during tests. did you remember to stub? error: %s", err))


Nice test failure for error case!

mislav · 2020-04-20T14:28:44Z

context/testing.go

+	}
+}
+
+func eq(t *testing.T, got interface{}, expected interface{}) {


Instead of having to import the testing package in a file that doesn't end in _test.go, could we describe t as simply conforming to an interface with only the Helper() method? Since this is the only method of t that we are invoking?

Or better yet, we can move the definition of eq into a _test.go file

mislav · 2020-04-20T14:29:55Z

command/config.go

+
+	err = cfg.Set(hostname, key, value)
+	if err != nil {
+		return fmt.Errorf("failed to set %q to %q: %s", key, value, err)


Nit: to legitimately wrap an error go 1.13-style, could we turn the %s into %w? I think that the error output will be the same

🤦‍♀️ I wasn't sure what the %w was until now and had avoided it out of principle. will switch to it now that I know better.

mislav · 2020-04-20T14:30:05Z

command/config.go

+
+	err = cfg.Write()
+	if err != nil {
+		return fmt.Errorf("failed to write config to disk: %s", err)


mislav · 2020-04-20T14:31:39Z

command/root.go

 	opts = append(opts, api.AddHeader("User-Agent", fmt.Sprintf("GitHub CLI %s", Version)))
 	if c, err := context.ParseDefaultConfig(); err == nil {
-		opts = append(opts, api.AddHeader("Authorization", fmt.Sprintf("token %s", c.Token)))
+		if token, err := c.Get(defaultHostname, "oauth_token"); err == nil {


In which case would there be an error here, and should we keep tolerating it?

it would have to be something messed up in the config and we shouldn't tolerate it; changed to fail

mislav · 2020-04-20T14:50:08Z

context/config_type.go

+const defaultGitProtocol = "https"
+
+// This interface describes interacting with some persistent configuration for gh.
+type Config interface {


Do you think it would be worth extracting, at the end, any of the config stuff from context to a new package under internal?

I do think that would be worthwhile. Having it context felt wrong and I wanted a config package; was just getting worried about how long this was taking. I'll see if I can do it quickly today.

vilmibm · 2020-04-20T19:00:45Z

@mislav caught up on all the feedback and also added stricter handling around blank user/oauth_token values.

vilmibm · 2020-04-20T22:44:40Z

just realized i screwed up default handling and need to fix it

mislav

Thank you for the update!

mislav · 2020-04-21T07:57:03Z

command/config.go

+	if err != nil {
+		panic("this should not happen")


Nit: I think it's perfectly fine to ignore the error here by assigning to _ instead of panicking

mislav · 2020-04-21T08:00:58Z

command/root.go

+	if token == "" || err != nil {
+		return nil, err


If token was an empty string but there was no error object, this will return nil, nil. If we want to return an explicit error for that case, we should describe it to ensure there is always an error object:

if err != nil { return nil, err } if token == "" { return nil, fmt.Errorf("...") }

good catch; i was going too fast

mislav

Great work!

tierninho · 2020-04-21T21:57:41Z

Nice work. I tested get and set for both the editor and git_protocol settings and they both work as expected. Also, any manual changes to the configs are respected by the commands.

Some things to consider for future iterations:

Allowing the user to revert to default settings.
Validation on the inputs as anything can be entered

Tested with d4dd4a6, OSX. (Will verify in Windows soon.)

vilmibm changed the title ~~[WIP] [EXTREME WIP] new config format + parsing~~ [WIP] new config format + parsing Apr 6, 2020

mislav reviewed Apr 7, 2020

View reviewed changes

vilmibm changed the title ~~[WIP] new config format + parsing~~ gh config Apr 10, 2020

vilmibm force-pushed the gh-config branch from 40f15f2 to 6885c1d Compare April 14, 2020 22:06

mislav reviewed Apr 17, 2020

View reviewed changes

new config infrastructure

a325db3

- adds config get and config set commands - supports arbitrary k/v strings set at top and host level - supports writing an updated config, preserving comments - supports mostly lazy evaluation of yaml

vilmibm force-pushed the gh-config branch from 8cf7811 to a325db3 Compare April 17, 2020 20:17

vilmibm marked this pull request as ready for review April 17, 2020 20:19

vilmibm added 2 commits April 17, 2020 15:21

clean up Config interface

a0b7f37

staunch some type bleeding

e823ad0

mislav suggested changes Apr 20, 2020

View reviewed changes

vilmibm added 6 commits April 20, 2020 12:22

favor %w over %s for error wrapping

bd907ec

usage cleanup

1a5bc06

do not import testing outside of a _test file

b1d5264

more strict erroring around missing user/token

815f461

move config stuff to its own package

c469307

linter appeasement

3220f2e

vilmibm requested a review from mislav April 20, 2020 19:00

oops

db9f4c3

mislav suggested changes Apr 21, 2020

View reviewed changes

vilmibm added 3 commits April 21, 2020 10:32

slopwip for defaults

bba6343

handle defaults at the Config level instead of ConfigMap

6e55e89

ignore MarkHidden errors

fcf2586

fix error handling

d4dd4a6

mislav approved these changes Apr 21, 2020

View reviewed changes

vilmibm merged commit 80d7513 into master Apr 21, 2020

tierninho self-requested a review April 21, 2020 21:42

vilmibm mentioned this pull request Apr 22, 2020

gh pr list failed on yaml unmarshal (cannot unmarshal !!map into []context.configEntry) #602

Closed

lcbm mentioned this pull request Apr 24, 2020

Update README #828

Merged

mislav deleted the gh-config branch June 25, 2020 14:37

		newConfigData := map[string]map[string]map[string]string{}
		newConfigData["hosts"] = map[string]map[string]string{}

		configForHost(string) (*HostConfig, error)
		parseHosts(yaml.Node) ([]HostConfig, error)

Conversation

vilmibm commented Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO TODID

Uh oh!

thefotios commented Apr 1, 2020

Uh oh!

vilmibm commented Apr 6, 2020

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mislav Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mislav Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vilmibm Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ampinsk commented Apr 7, 2020

Uh oh!

vilmibm commented Apr 7, 2020

Uh oh!

ampinsk commented Apr 7, 2020

Uh oh!

mislav left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vilmibm Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mislav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mislav Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vilmibm commented Apr 1, 2020 •

edited

Loading

mislav Apr 7, 2020 •

edited

Loading

mislav Apr 7, 2020 •

edited

Loading

vilmibm Apr 8, 2020 •

edited

Loading

mislav left a comment •

edited

Loading

vilmibm Apr 17, 2020 •

edited

Loading

mislav Apr 20, 2020 •

edited

Loading