From 43a8b311bc4d90aee5950b5c66866f583264a5f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 27 May 2020 18:18:41 +0200 Subject: [PATCH 01/14] Un-publish `Hosts` from the Config interface It's not being depended on anywhere right now, so this minimizes the public interface. --- internal/config/config_type.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/config/config_type.go b/internal/config/config_type.go index e37c3e884..6517e1be3 100644 --- a/internal/config/config_type.go +++ b/internal/config/config_type.go @@ -12,7 +12,6 @@ const defaultGitProtocol = "https" // This interface describes interacting with some persistent configuration for gh. type Config interface { - Hosts() ([]*HostConfig, error) Get(string, string) (string, error) Set(string, string, string) error Write() error @@ -145,7 +144,7 @@ func (c *fileConfig) Set(hostname, key, value string) error { } func (c *fileConfig) configForHost(hostname string) (*HostConfig, error) { - hosts, err := c.Hosts() + hosts, err := c.hostEntries() if err != nil { return nil, fmt.Errorf("failed to parse hosts config: %w", err) } @@ -167,7 +166,7 @@ func (c *fileConfig) Write() error { return WriteConfigFile(ConfigFile(), marshalled) } -func (c *fileConfig) Hosts() ([]*HostConfig, error) { +func (c *fileConfig) hostEntries() ([]*HostConfig, error) { if len(c.hosts) > 0 { return c.hosts, nil } From f42c9d4b2d61f3c898376e3dec25a28eecdf49f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 27 May 2020 19:56:56 +0200 Subject: [PATCH 02/14] Allow stubbing multiple config files --- command/config_test.go | 8 +++---- command/testing.go | 2 +- context/blank_context.go | 2 +- internal/config/config_file_test.go | 16 +++++++------- internal/config/testing.go | 33 ++++++++++++++++++++++++----- 5 files changed, 42 insertions(+), 19 deletions(-) diff --git a/command/config_test.go b/command/config_test.go index 00148f3b4..61f89ddf8 100644 --- a/command/config_test.go +++ b/command/config_test.go @@ -50,7 +50,7 @@ func TestConfigSet(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") buf := bytes.NewBufferString("") - defer config.StubWriteConfig(buf)() + defer config.StubWriteConfig(buf, nil)() output, err := RunCommand("config set editor ed") if err != nil { t.Fatalf("error running command `config set editor ed`: %v", err) @@ -80,7 +80,7 @@ editor: ed initBlankContext(cfg, "OWNER/REPO", "master") buf := bytes.NewBufferString("") - defer config.StubWriteConfig(buf)() + defer config.StubWriteConfig(buf, nil)() output, err := RunCommand("config set editor vim") if err != nil { @@ -142,7 +142,7 @@ func TestConfigSetHost(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") buf := bytes.NewBufferString("") - defer config.StubWriteConfig(buf)() + defer config.StubWriteConfig(buf, nil)() output, err := RunCommand("config set -hgithub.com git_protocol ssh") if err != nil { t.Fatalf("error running command `config set editor ed`: %v", err) @@ -172,7 +172,7 @@ hosts: initBlankContext(cfg, "OWNER/REPO", "master") buf := bytes.NewBufferString("") - defer config.StubWriteConfig(buf)() + defer config.StubWriteConfig(buf, nil)() output, err := RunCommand("config set -hgithub.com git_protocol https") if err != nil { diff --git a/command/testing.go b/command/testing.go index 5612f58ea..2ca2ad19e 100644 --- a/command/testing.go +++ b/command/testing.go @@ -88,7 +88,7 @@ func initBlankContext(cfg, repo, branch string) { // NOTE we are not restoring the original readConfig; we never want to touch the config file on // disk during tests. - config.StubConfig(cfg) + config.StubConfig(cfg, "") return ctx } diff --git a/context/blank_context.go b/context/blank_context.go index ed8784cfc..3ea657abe 100644 --- a/context/blank_context.go +++ b/context/blank_context.go @@ -23,7 +23,7 @@ type blankContext struct { } func (c *blankContext) Config() (config.Config, error) { - cfg, err := config.ParseConfig("boom.txt") + cfg, err := config.ParseConfig("config.yml") if err != nil { panic(fmt.Sprintf("failed to parse config during tests. did you remember to stub? error: %s", err)) } diff --git a/internal/config/config_file_test.go b/internal/config/config_file_test.go index 4ddd63435..6af10d0e1 100644 --- a/internal/config/config_file_test.go +++ b/internal/config/config_file_test.go @@ -22,8 +22,8 @@ hosts: github.com: user: monalisa oauth_token: OTOKEN -`)() - config, err := ParseConfig("filename") +`, "")() + config, err := ParseConfig("config.yml") eq(t, err, nil) user, err := config.Get("github.com", "user") eq(t, err, nil) @@ -42,8 +42,8 @@ hosts: github.com: user: monalisa oauth_token: OTOKEN -`)() - config, err := ParseConfig("filename") +`, "")() + config, err := ParseConfig("config.yml") eq(t, err, nil) user, err := config.Get("github.com", "user") eq(t, err, nil) @@ -59,8 +59,8 @@ hosts: example.com: user: wronguser oauth_token: NOTTHIS -`)() - config, err := ParseConfig("filename") +`, "")() + config, err := ParseConfig("config.yml") eq(t, err, nil) _, err = config.Get("github.com", "user") eq(t, err, errors.New(`could not find config entry for "github.com"`)) @@ -79,11 +79,11 @@ github.com: } buf := bytes.NewBufferString("") - defer StubWriteConfig(buf)() + defer StubWriteConfig(buf, nil)() defer StubBackupConfig()() - err = migrateConfig("boom.txt", &root) + err = migrateConfig("config.yml", &root) eq(t, err, nil) expected := `hosts: diff --git a/internal/config/testing.go b/internal/config/testing.go index a91cedfae..59c2ff212 100644 --- a/internal/config/testing.go +++ b/internal/config/testing.go @@ -1,7 +1,10 @@ package config import ( + "fmt" "io" + "os" + "path" ) func StubBackupConfig() func() { @@ -15,21 +18,41 @@ func StubBackupConfig() func() { } } -func StubWriteConfig(w io.Writer) func() { +func StubWriteConfig(wc io.Writer, wh io.Writer) func() { orig := WriteConfigFile WriteConfigFile = func(fn string, data []byte) error { - _, err := w.Write(data) - return err + switch path.Base(fn) { + case "config.yml": + _, err := wc.Write(data) + return err + case "hosts.yml": + _, err := wh.Write(data) + return err + default: + return fmt.Errorf("write to unstubbed file: %q", fn) + } } return func() { WriteConfigFile = orig } } -func StubConfig(content string) func() { +func StubConfig(main, hosts string) func() { orig := ReadConfigFile ReadConfigFile = func(fn string) ([]byte, error) { - return []byte(content), nil + switch path.Base(fn) { + case "config.yml": + return []byte(main), nil + case "hosts.yml": + if hosts == "" { + return []byte(nil), os.ErrNotExist + } else { + return []byte(hosts), nil + } + default: + return []byte(nil), fmt.Errorf("read from unstubbed file: %q", fn) + } + } return func() { ReadConfigFile = orig From fd7b87f3fa0263cbd70feea93dee75b9fc4595e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 27 May 2020 21:01:01 +0200 Subject: [PATCH 03/14] Allow writing host-specific keys in a blank config --- internal/config/config_file.go | 8 +++++- internal/config/config_file_test.go | 2 +- internal/config/config_setup.go | 42 ++++++---------------------- internal/config/config_type.go | 43 +++++++++++++++++++++++++++-- internal/config/config_type_test.go | 29 +++++++++++++++++++ 5 files changed, 86 insertions(+), 38 deletions(-) create mode 100644 internal/config/config_type_test.go diff --git a/internal/config/config_file.go b/internal/config/config_file.go index 2670aff20..67304eb8f 100644 --- a/internal/config/config_file.go +++ b/internal/config/config_file.go @@ -7,6 +7,7 @@ import ( "io/ioutil" "os" "path" + "path/filepath" "github.com/mitchellh/go-homedir" "gopkg.in/yaml.v3" @@ -49,7 +50,12 @@ var ReadConfigFile = func(fn string) ([]byte, error) { } var WriteConfigFile = func(fn string, data []byte) error { - cfgFile, err := os.OpenFile(ConfigFile(), os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0600) // cargo coded from setup + err := os.MkdirAll(filepath.Dir(fn), 0771) + if err != nil { + return err + } + + cfgFile, err := os.OpenFile(fn, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0600) // cargo coded from setup if err != nil { return err } diff --git a/internal/config/config_file_test.go b/internal/config/config_file_test.go index 6af10d0e1..28d50a37e 100644 --- a/internal/config/config_file_test.go +++ b/internal/config/config_file_test.go @@ -63,7 +63,7 @@ hosts: config, err := ParseConfig("config.yml") eq(t, err, nil) _, err = config.Get("github.com", "user") - eq(t, err, errors.New(`could not find config entry for "github.com"`)) + eq(t, err, &NotFoundError{errors.New(`could not find config entry for "github.com"`)}) } func Test_migrateConfig(t *testing.T) { diff --git a/internal/config/config_setup.go b/internal/config/config_setup.go index 2fc414a1b..17dbcb190 100644 --- a/internal/config/config_setup.go +++ b/internal/config/config_setup.go @@ -5,12 +5,10 @@ import ( "fmt" "io" "os" - "path/filepath" "strings" "github.com/cli/cli/api" "github.com/cli/cli/auth" - "gopkg.in/yaml.v3" ) const ( @@ -76,44 +74,20 @@ func setupConfigFile(filename string) (Config, error) { return nil, err } - // TODO this sucks. It precludes us laying out a nice config with comments and such. - type yamlConfig struct { - Hosts map[string]map[string]string + cfg := NewBlankConfig() + err = cfg.Set(oauthHost, "user", userLogin) + if err != nil { + return nil, err } - - yamlHosts := map[string]map[string]string{} - yamlHosts[oauthHost] = map[string]string{} - yamlHosts[oauthHost]["user"] = userLogin - yamlHosts[oauthHost]["oauth_token"] = token - - defaultConfig := yamlConfig{ - Hosts: yamlHosts, - } - - err = os.MkdirAll(filepath.Dir(filename), 0771) + err = cfg.Set(oauthHost, "oauth_token", token) if err != nil { return nil, err } - cfgFile, err := os.OpenFile(filename, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0600) - if err != nil { - return nil, err + if err = cfg.Write(); err == nil { + AuthFlowComplete() } - defer cfgFile.Close() - - yamlData, err := yaml.Marshal(defaultConfig) - if err != nil { - return nil, err - } - _, err = cfgFile.Write(yamlData) - if err != nil { - return nil, err - } - - AuthFlowComplete() - - // TODO cleaner error handling? this "should" always work given that we /just/ wrote the file... - return ParseConfig(filename) + return cfg, err } func getViewer(token string) (string, error) { diff --git a/internal/config/config_type.go b/internal/config/config_type.go index 6517e1be3..4c38c421c 100644 --- a/internal/config/config_type.go +++ b/internal/config/config_type.go @@ -88,6 +88,13 @@ func NewConfig(root *yaml.Node) Config { } } +func NewBlankConfig() Config { + return NewConfig(&yaml.Node{ + Kind: yaml.DocumentNode, + Content: []*yaml.Node{{Kind: yaml.MappingNode}}, + }) +} + // This type implements a Config interface and represents a config file on disk. type fileConfig struct { ConfigMap @@ -136,7 +143,10 @@ func (c *fileConfig) Set(hostname, key, value string) error { return c.SetStringValue(key, value) } else { hostCfg, err := c.configForHost(hostname) - if err != nil { + var notFound *NotFoundError + if errors.As(err, ¬Found) { + hostCfg = c.makeConfigForHost(hostname) + } else if err != nil { return err } return hostCfg.SetStringValue(key, value) @@ -154,7 +164,7 @@ func (c *fileConfig) configForHost(hostname string) (*HostConfig, error) { return hc, nil } } - return nil, fmt.Errorf("could not find config entry for %q", hostname) + return nil, &NotFoundError{fmt.Errorf("could not find config entry for %q", hostname)} } func (c *fileConfig) Write() error { @@ -186,6 +196,35 @@ func (c *fileConfig) hostEntries() ([]*HostConfig, error) { return hostConfigs, nil } +func (c *fileConfig) makeConfigForHost(hostname string) *HostConfig { + hostRoot := &yaml.Node{Kind: yaml.MappingNode} + hostCfg := &HostConfig{ + Host: hostname, + ConfigMap: ConfigMap{Root: hostRoot}, + } + + var notFound *NotFoundError + _, hostsEntry, err := c.FindEntry("hosts") + if errors.As(err, ¬Found) { + hostsEntry = &yaml.Node{Kind: yaml.MappingNode} + c.Root.Content = append(c.Root.Content, + &yaml.Node{ + Kind: yaml.ScalarNode, + Value: "hosts", + }, hostsEntry) + } else if err != nil { + panic(err) + } + + hostsEntry.Content = append(hostsEntry.Content, + &yaml.Node{ + Kind: yaml.ScalarNode, + Value: hostname, + }, hostRoot) + + return hostCfg +} + func (c *fileConfig) parseHosts(hostsEntry *yaml.Node) ([]*HostConfig, error) { hostConfigs := []*HostConfig{} diff --git a/internal/config/config_type_test.go b/internal/config/config_type_test.go new file mode 100644 index 000000000..eb0747571 --- /dev/null +++ b/internal/config/config_type_test.go @@ -0,0 +1,29 @@ +package config + +import ( + "bytes" + "testing" + + "github.com/stretchr/testify/assert" +) + +func Test_fileConfig_Set(t *testing.T) { + cb := bytes.Buffer{} + StubWriteConfig(&cb, nil) + + c := NewBlankConfig() + assert.NoError(t, c.Set("", "editor", "nano")) + assert.NoError(t, c.Set("github.com", "git_protocol", "ssh")) + assert.NoError(t, c.Set("example.com", "editor", "vim")) + assert.NoError(t, c.Set("github.com", "user", "hubot")) + assert.NoError(t, c.Write()) + + assert.Equal(t, `editor: nano +hosts: + github.com: + git_protocol: ssh + user: hubot + example.com: + editor: vim +`, cb.String()) +} From 77227a6c50c0d9ab6118d8d07548b2f3415723e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 27 May 2020 21:45:52 +0200 Subject: [PATCH 04/14] Trigger OAuth flow only when requesting auth token Previously we would trigger OAuth flow when the config file did not exist. Now we will let an empty Config object be initialized in that case, but trigger OAuth flow when the Context caller requests an AuthToken. --- command/root.go | 27 ++++------------- context/context.go | 15 +++++++--- internal/config/config_file.go | 8 ----- internal/config/config_setup.go | 53 ++++++++++++++++----------------- 4 files changed, 41 insertions(+), 62 deletions(-) diff --git a/command/root.go b/command/root.go index 208c7814d..41dd5f81e 100644 --- a/command/root.go +++ b/command/root.go @@ -180,24 +180,16 @@ var apiClientForContext = func(ctx context.Context) (*api.Client, error) { checkScopesFunc := func(appID string) error { if config.IsGitHubApp(appID) && !tokenFromEnv() && utils.IsTerminal(os.Stdin) && utils.IsTerminal(os.Stderr) { - newToken, loginHandle, err := config.AuthFlow("Notice: additional authorization required") - if err != nil { - return err - } cfg, err := ctx.Config() if err != nil { return err } - _ = cfg.Set(defaultHostname, "oauth_token", newToken) - _ = cfg.Set(defaultHostname, "user", loginHandle) - // update config file on disk - err = cfg.Write() + newToken, err := config.AuthFlowWithConfig(cfg, defaultHostname, "Notice: additional authorization required") if err != nil { return err } // update configuration in memory token = newToken - config.AuthFlowComplete() } else { fmt.Fprintln(os.Stderr, "Warning: gh now requires the `read:org` OAuth scope.") fmt.Fprintln(os.Stderr, "Visit https://github.com/settings/tokens and edit your token to enable `read:org`") @@ -234,28 +226,19 @@ var ensureScopes = func(ctx context.Context, client *api.Client, wantedScopes .. tokenFromEnv := len(os.Getenv("GITHUB_TOKEN")) > 0 if config.IsGitHubApp(appID) && !tokenFromEnv && utils.IsTerminal(os.Stdin) && utils.IsTerminal(os.Stderr) { - newToken, loginHandle, err := config.AuthFlow("Notice: additional authorization required") - if err != nil { - return client, err - } cfg, err := ctx.Config() if err != nil { - return client, err + return nil, err } - _ = cfg.Set(defaultHostname, "oauth_token", newToken) - _ = cfg.Set(defaultHostname, "user", loginHandle) - // update config file on disk - err = cfg.Write() + _, err = config.AuthFlowWithConfig(cfg, defaultHostname, "Notice: additional authorization required") if err != nil { - return client, err + return nil, err } - // update configuration in memory - config.AuthFlowComplete() + reloadedClient, err := apiClientForContext(ctx) if err != nil { return client, err } - return reloadedClient, nil } else { fmt.Fprintln(os.Stderr, fmt.Sprintf("Warning: gh now requires %s OAuth scopes.", wantedScopes)) diff --git a/context/context.go b/context/context.go index ddeb82c4d..236f9e722 100644 --- a/context/context.go +++ b/context/context.go @@ -3,6 +3,7 @@ package context import ( "errors" "fmt" + "os" "sort" "github.com/cli/cli/api" @@ -162,11 +163,13 @@ type fsContext struct { func (c *fsContext) Config() (config.Config, error) { if c.config == nil { - config, err := config.ParseOrSetupConfigFile(config.ConfigFile()) - if err != nil { + cfg, err := config.ParseDefaultConfig() + if errors.Is(err, os.ErrNotExist) { + cfg = config.NewBlankConfig() + } else if err != nil { return nil, err } - c.config = config + c.config = cfg c.authToken = "" } return c.config, nil @@ -182,8 +185,12 @@ func (c *fsContext) AuthToken() (string, error) { return "", err } + var notFound *config.NotFoundError token, err := cfg.Get(defaultHostname, "oauth_token") - if token == "" || err != nil { + if token == "" || errors.As(err, ¬Found) { + // interactive OAuth flow + return config.AuthFlowWithConfig(cfg, defaultHostname, "Notice: authentication required") + } else if err != nil { return "", err } diff --git a/internal/config/config_file.go b/internal/config/config_file.go index 67304eb8f..720b86b6e 100644 --- a/internal/config/config_file.go +++ b/internal/config/config_file.go @@ -22,14 +22,6 @@ func ConfigFile() string { return path.Join(ConfigDir(), "config.yml") } -func ParseOrSetupConfigFile(fn string) (Config, error) { - config, err := ParseConfig(fn) - if err != nil && errors.Is(err, os.ErrNotExist) { - return setupConfigFile(fn) - } - return config, err -} - func ParseDefaultConfig() (Config, error) { return ParseConfig(ConfigFile()) } diff --git a/internal/config/config_setup.go b/internal/config/config_setup.go index 17dbcb190..5bb5f694f 100644 --- a/internal/config/config_setup.go +++ b/internal/config/config_setup.go @@ -11,10 +11,6 @@ import ( "github.com/cli/cli/auth" ) -const ( - oauthHost = "github.com" -) - var ( // The "GitHub CLI" OAuth app oauthClientID = "178c6fc778ccc68e1d6a" @@ -29,7 +25,31 @@ func IsGitHubApp(id string) bool { return id == "178c6fc778ccc68e1d6a" || id == "4d747ba5675d5d66553f" } -func AuthFlow(notice string) (string, string, error) { +func AuthFlowWithConfig(cfg Config, hostname, notice string) (string, error) { + token, userLogin, err := AuthFlow(hostname, notice) + if err != nil { + return "", err + } + + err = cfg.Set(hostname, "user", userLogin) + if err != nil { + return "", err + } + err = cfg.Set(hostname, "oauth_token", token) + if err != nil { + return "", err + } + + err = cfg.Write() + if err != nil { + return "", err + } + + AuthFlowComplete() + return token, nil +} + +func AuthFlow(oauthHost, notice string) (string, string, error) { var verboseStream io.Writer if strings.Contains(os.Getenv("DEBUG"), "oauth") { verboseStream = os.Stderr @@ -67,29 +87,6 @@ func AuthFlowComplete() { _ = waitForEnter(os.Stdin) } -// FIXME: make testable -func setupConfigFile(filename string) (Config, error) { - token, userLogin, err := AuthFlow("Notice: authentication required") - if err != nil { - return nil, err - } - - cfg := NewBlankConfig() - err = cfg.Set(oauthHost, "user", userLogin) - if err != nil { - return nil, err - } - err = cfg.Set(oauthHost, "oauth_token", token) - if err != nil { - return nil, err - } - - if err = cfg.Write(); err == nil { - AuthFlowComplete() - } - return cfg, err -} - func getViewer(token string) (string, error) { http := api.NewClient(api.AddHeader("Authorization", fmt.Sprintf("token %s", token))) return api.CurrentLoginName(http) From 1595d3b950cea7f62fbe1ca983e6b1edf2fdbc30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 27 May 2020 22:11:36 +0200 Subject: [PATCH 05/14] Handle HTTP errors in HasScopes --- api/client.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/api/client.go b/api/client.go index fe1fcf842..b8c6857fb 100644 --- a/api/client.go +++ b/api/client.go @@ -165,6 +165,10 @@ func (c Client) HasScopes(wantedScopes ...string) (bool, string, error) { } defer res.Body.Close() + if res.StatusCode != 200 { + return false, "", handleHTTPError(res) + } + appID := res.Header.Get("X-Oauth-Client-Id") hasScopes := strings.Split(res.Header.Get("X-Oauth-Scopes"), ",") From d6f58fb448f5b79abd6c6c88c4088723d0fc49f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 27 May 2020 22:19:47 +0200 Subject: [PATCH 06/14] :fire: hosts optimization We dynamically add hosts on `Set`, so this `hosts` cache might fall out of date. We could ensure to keep it updated, but I'm not convinced it's necessary for speed right now. --- internal/config/config_type.go | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/internal/config/config_type.go b/internal/config/config_type.go index 4c38c421c..b45ca554b 100644 --- a/internal/config/config_type.go +++ b/internal/config/config_type.go @@ -99,7 +99,6 @@ func NewBlankConfig() Config { type fileConfig struct { ConfigMap documentRoot *yaml.Node - hosts []*HostConfig } func (c *fileConfig) Get(hostname, key string) (string, error) { @@ -177,23 +176,12 @@ func (c *fileConfig) Write() error { } func (c *fileConfig) hostEntries() ([]*HostConfig, error) { - if len(c.hosts) > 0 { - return c.hosts, nil - } - _, hostsEntry, err := c.FindEntry("hosts") if err != nil { return nil, fmt.Errorf("could not find hosts config: %w", err) } - hostConfigs, err := c.parseHosts(hostsEntry) - if err != nil { - return nil, fmt.Errorf("could not parse hosts config: %w", err) - } - - c.hosts = hostConfigs - - return hostConfigs, nil + return c.parseHosts(hostsEntry) } func (c *fileConfig) makeConfigForHost(hostname string) *HostConfig { From bad138e448458e3cb95a4fc0d96e441513cd36a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 27 May 2020 22:48:01 +0200 Subject: [PATCH 07/14] Enable reading from and writing to empty config files --- internal/config/config_file.go | 7 +++++-- internal/config/config_file_test.go | 14 ++++++++++++++ internal/config/config_type.go | 5 +++++ internal/config/config_type_test.go | 10 ++++++++++ 4 files changed, 34 insertions(+), 2 deletions(-) diff --git a/internal/config/config_file.go b/internal/config/config_file.go index 720b86b6e..ff10d446f 100644 --- a/internal/config/config_file.go +++ b/internal/config/config_file.go @@ -76,8 +76,11 @@ func parseConfigFile(fn string) ([]byte, *yaml.Node, error) { if err != nil { return data, nil, err } - if len(root.Content) < 1 { - return data, &root, fmt.Errorf("malformed config") + if len(root.Content) == 0 { + return data, &yaml.Node{ + Kind: yaml.DocumentNode, + Content: []*yaml.Node{{Kind: yaml.MappingNode}}, + }, nil } if root.Content[0].Kind != yaml.MappingNode { return data, &root, fmt.Errorf("expected a top level map") diff --git a/internal/config/config_file_test.go b/internal/config/config_file_test.go index 28d50a37e..441dc22f2 100644 --- a/internal/config/config_file_test.go +++ b/internal/config/config_file_test.go @@ -3,6 +3,7 @@ package config import ( "bytes" "errors" + "fmt" "reflect" "testing" @@ -94,3 +95,16 @@ github.com: eq(t, buf.String(), expected) } + +func Test_parseConfigFile(t *testing.T) { + fileContents := []string{"", " ", "\n"} + for _, contents := range fileContents { + t.Run(fmt.Sprintf("contents: %q", contents), func(t *testing.T) { + defer StubConfig(contents, "")() + _, yamlRoot, err := parseConfigFile("config.yml") + eq(t, err, nil) + eq(t, yamlRoot.Content[0].Kind, yaml.MappingNode) + eq(t, len(yamlRoot.Content[0].Content), 0) + }) + } +} diff --git a/internal/config/config_type.go b/internal/config/config_type.go index b45ca554b..c2fc66d9b 100644 --- a/internal/config/config_type.go +++ b/internal/config/config_type.go @@ -1,6 +1,7 @@ package config import ( + "bytes" "errors" "fmt" @@ -172,6 +173,10 @@ func (c *fileConfig) Write() error { return err } + if bytes.Equal(marshalled, []byte("{}\n")) { + marshalled = []byte{} + } + return WriteConfigFile(ConfigFile(), marshalled) } diff --git a/internal/config/config_type_test.go b/internal/config/config_type_test.go index eb0747571..97ffd9aa7 100644 --- a/internal/config/config_type_test.go +++ b/internal/config/config_type_test.go @@ -27,3 +27,13 @@ hosts: editor: vim `, cb.String()) } + +func Test_fileConfig_Write(t *testing.T) { + cb := bytes.Buffer{} + StubWriteConfig(&cb, nil) + + c := NewBlankConfig() + assert.NoError(t, c.Write()) + + assert.Equal(t, "", cb.String()) +} From c08d4f0697b20d5334ff0ce55b229089cffbaeb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 27 May 2020 23:24:20 +0200 Subject: [PATCH 08/14] Write all per-host config entries to `hosts.yml` Read from and write to the `hosts.yml` file every time `config.yml` is accessed. Everything that before went under the `hosts:` map now belongs to `hosts.yml`. --- command/config_test.go | 106 ++++++++++++++++++---------- command/testing.go | 2 +- internal/config/config_file.go | 101 ++++++++++++-------------- internal/config/config_file_test.go | 48 ++++++++----- internal/config/config_type.go | 35 +++++++-- internal/config/config_type_test.go | 28 ++++---- 6 files changed, 191 insertions(+), 129 deletions(-) diff --git a/command/config_test.go b/command/config_test.go index 61f89ddf8..53654f54f 100644 --- a/command/config_test.go +++ b/command/config_test.go @@ -49,23 +49,31 @@ func TestConfigGet_not_found(t *testing.T) { func TestConfigSet(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") - buf := bytes.NewBufferString("") - defer config.StubWriteConfig(buf, nil)() + mainBuf := bytes.Buffer{} + hostsBuf := bytes.Buffer{} + defer config.StubWriteConfig(&mainBuf, &hostsBuf)() + output, err := RunCommand("config set editor ed") if err != nil { t.Fatalf("error running command `config set editor ed`: %v", err) } - eq(t, output.String(), "") + if len(output.String()) > 0 { + t.Errorf("expected output to be blank: %q", output.String()) + } - expected := `hosts: - github.com: - user: OWNER - oauth_token: 1234567890 -editor: ed + expectedMain := "editor: ed\n" + expectedHosts := `github.com: + user: OWNER + oauth_token: "1234567890" ` - eq(t, buf.String(), expected) + if mainBuf.String() != expectedMain { + t.Errorf("expected config.yml to be %q, got %q", expectedMain, mainBuf.String()) + } + if hostsBuf.String() != expectedHosts { + t.Errorf("expected hosts.yml to be %q, got %q", expectedHosts, hostsBuf.String()) + } } func TestConfigSet_update(t *testing.T) { @@ -79,23 +87,31 @@ editor: ed initBlankContext(cfg, "OWNER/REPO", "master") - buf := bytes.NewBufferString("") - defer config.StubWriteConfig(buf, nil)() + mainBuf := bytes.Buffer{} + hostsBuf := bytes.Buffer{} + defer config.StubWriteConfig(&mainBuf, &hostsBuf)() output, err := RunCommand("config set editor vim") if err != nil { t.Fatalf("error running command `config get editor`: %v", err) } - eq(t, output.String(), "") + if len(output.String()) > 0 { + t.Errorf("expected output to be blank: %q", output.String()) + } - expected := `hosts: - github.com: - user: OWNER - oauth_token: MUSTBEHIGHCUZIMATOKEN -editor: vim + expectedMain := "editor: vim\n" + expectedHosts := `github.com: + user: OWNER + oauth_token: MUSTBEHIGHCUZIMATOKEN ` - eq(t, buf.String(), expected) + + if mainBuf.String() != expectedMain { + t.Errorf("expected config.yml to be %q, got %q", expectedMain, mainBuf.String()) + } + if hostsBuf.String() != expectedHosts { + t.Errorf("expected hosts.yml to be %q, got %q", expectedHosts, hostsBuf.String()) + } } func TestConfigGetHost(t *testing.T) { @@ -141,23 +157,32 @@ git_protocol: ssh func TestConfigSetHost(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") - buf := bytes.NewBufferString("") - defer config.StubWriteConfig(buf, nil)() + mainBuf := bytes.Buffer{} + hostsBuf := bytes.Buffer{} + defer config.StubWriteConfig(&mainBuf, &hostsBuf)() + output, err := RunCommand("config set -hgithub.com git_protocol ssh") if err != nil { t.Fatalf("error running command `config set editor ed`: %v", err) } - eq(t, output.String(), "") + if len(output.String()) > 0 { + t.Errorf("expected output to be blank: %q", output.String()) + } - expected := `hosts: - github.com: - user: OWNER - oauth_token: 1234567890 - git_protocol: ssh + expectedMain := "" + expectedHosts := `github.com: + user: OWNER + oauth_token: "1234567890" + git_protocol: ssh ` - eq(t, buf.String(), expected) + if mainBuf.String() != expectedMain { + t.Errorf("expected config.yml to be %q, got %q", expectedMain, mainBuf.String()) + } + if hostsBuf.String() != expectedHosts { + t.Errorf("expected hosts.yml to be %q, got %q", expectedHosts, hostsBuf.String()) + } } func TestConfigSetHost_update(t *testing.T) { @@ -171,21 +196,30 @@ hosts: initBlankContext(cfg, "OWNER/REPO", "master") - buf := bytes.NewBufferString("") - defer config.StubWriteConfig(buf, nil)() + mainBuf := bytes.Buffer{} + hostsBuf := bytes.Buffer{} + defer config.StubWriteConfig(&mainBuf, &hostsBuf)() output, err := RunCommand("config set -hgithub.com git_protocol https") if err != nil { t.Fatalf("error running command `config get editor`: %v", err) } - eq(t, output.String(), "") + if len(output.String()) > 0 { + t.Errorf("expected output to be blank: %q", output.String()) + } - expected := `hosts: - github.com: - git_protocol: https - user: OWNER - oauth_token: MUSTBEHIGHCUZIMATOKEN + expectedMain := "" + expectedHosts := `github.com: + git_protocol: https + user: OWNER + oauth_token: MUSTBEHIGHCUZIMATOKEN ` - eq(t, buf.String(), expected) + + if mainBuf.String() != expectedMain { + t.Errorf("expected config.yml to be %q, got %q", expectedMain, mainBuf.String()) + } + if hostsBuf.String() != expectedHosts { + t.Errorf("expected hosts.yml to be %q, got %q", expectedHosts, hostsBuf.String()) + } } diff --git a/command/testing.go b/command/testing.go index 2ca2ad19e..af713aa29 100644 --- a/command/testing.go +++ b/command/testing.go @@ -19,7 +19,7 @@ import ( const defaultTestConfig = `hosts: github.com: user: OWNER - oauth_token: 1234567890 + oauth_token: "1234567890" ` type askStubber struct { diff --git a/internal/config/config_file.go b/internal/config/config_file.go index ff10d446f..71fffb0d1 100644 --- a/internal/config/config_file.go +++ b/internal/config/config_file.go @@ -7,7 +7,6 @@ import ( "io/ioutil" "os" "path" - "path/filepath" "github.com/mitchellh/go-homedir" "gopkg.in/yaml.v3" @@ -22,6 +21,10 @@ func ConfigFile() string { return path.Join(ConfigDir(), "config.yml") } +func hostsConfigFile(fn string) string { + return path.Join(path.Dir(fn), "hosts.yml") +} + func ParseDefaultConfig() (Config, error) { return ParseConfig(ConfigFile()) } @@ -42,7 +45,7 @@ var ReadConfigFile = func(fn string) ([]byte, error) { } var WriteConfigFile = func(fn string, data []byte) error { - err := os.MkdirAll(filepath.Dir(fn), 0771) + err := os.MkdirAll(path.Dir(fn), 0771) if err != nil { return err } @@ -91,73 +94,44 @@ func parseConfigFile(fn string) ([]byte, *yaml.Node, error) { func isLegacy(root *yaml.Node) bool { for _, v := range root.Content[0].Content { - if v.Value == "hosts" { - return false + if v.Value == "github.com" { + return true } } - return true + return false } -func migrateConfig(fn string, root *yaml.Node) error { - type ConfigEntry map[string]string - type ConfigHash map[string]ConfigEntry - - newConfigData := map[string]ConfigHash{} - newConfigData["hosts"] = ConfigHash{} - - topLevelKeys := root.Content[0].Content - - for i, x := range topLevelKeys { - if x.Value == "" { - continue - } - if i+1 == len(topLevelKeys) { - break - } - hostname := x.Value - newConfigData["hosts"][hostname] = ConfigEntry{} - - authKeys := topLevelKeys[i+1].Content[0].Content - - for j, y := range authKeys { - if j+1 == len(authKeys) { - break - } - switch y.Value { - case "user": - newConfigData["hosts"][hostname]["user"] = authKeys[j+1].Value - case "oauth_token": - newConfigData["hosts"][hostname]["oauth_token"] = authKeys[j+1].Value - } - } - } - - if _, ok := newConfigData["hosts"][defaultHostname]; !ok { - return errors.New("could not find default host configuration") - } - - defaultHostConfig := newConfigData["hosts"][defaultHostname] - - if _, ok := defaultHostConfig["user"]; !ok { - return errors.New("default host configuration missing user") - } - - if _, ok := defaultHostConfig["oauth_token"]; !ok { - return errors.New("default host configuration missing oauth_token") - } - - newConfig, err := yaml.Marshal(newConfigData) +func migrateConfig(fn string) error { + b, err := ReadConfigFile(fn) if err != nil { return err } + var hosts map[string][]map[string]string + err = yaml.Unmarshal(b, &hosts) + if err != nil { + return fmt.Errorf("error decoding legacy format: %w", err) + } + + cfg := NewBlankConfig() + for hostname, entries := range hosts { + if len(entries) < 1 { + continue + } + for key, value := range entries[0] { + if err := cfg.Set(hostname, key, value); err != nil { + return err + } + } + } + err = BackupConfigFile(fn) if err != nil { return fmt.Errorf("failed to back up existing config: %w", err) } - return WriteConfigFile(fn, newConfig) + return cfg.Write() } func ParseConfig(fn string) (Config, error) { @@ -167,15 +141,28 @@ func ParseConfig(fn string) (Config, error) { } if isLegacy(root) { - err = migrateConfig(fn, root) + err = migrateConfig(fn) if err != nil { - return nil, err + return nil, fmt.Errorf("error migrating legacy config: %w", err) } _, root, err = parseConfigFile(fn) if err != nil { return nil, fmt.Errorf("failed to reparse migrated config: %w", err) } + } else { + if _, hostsRoot, err := parseConfigFile(hostsConfigFile(fn)); err == nil { + if len(hostsRoot.Content[0].Content) > 0 { + newContent := []*yaml.Node{ + {Value: "hosts"}, + hostsRoot.Content[0], + } + restContent := root.Content[0].Content + root.Content[0].Content = append(newContent, restContent...) + } + } else if !errors.Is(err, os.ErrNotExist) { + return nil, err + } } return NewConfig(root), nil diff --git a/internal/config/config_file_test.go b/internal/config/config_file_test.go index 441dc22f2..edc3a6ae9 100644 --- a/internal/config/config_file_test.go +++ b/internal/config/config_file_test.go @@ -54,6 +54,22 @@ hosts: eq(t, token, "OTOKEN") } +func Test_parseConfig_hostsFile(t *testing.T) { + defer StubConfig("", `--- +github.com: + user: monalisa + oauth_token: OTOKEN +`)() + config, err := ParseConfig("config.yml") + eq(t, err, nil) + user, err := config.Get("github.com", "user") + eq(t, err, nil) + eq(t, user, "monalisa") + token, err := config.Get("github.com", "oauth_token") + eq(t, err, nil) + eq(t, token, "OTOKEN") +} + func Test_parseConfig_notFound(t *testing.T) { defer StubConfig(`--- hosts: @@ -67,33 +83,29 @@ hosts: eq(t, err, &NotFoundError{errors.New(`could not find config entry for "github.com"`)}) } -func Test_migrateConfig(t *testing.T) { - oldStyle := `--- +func Test_ParseConfig_migrateConfig(t *testing.T) { + defer StubConfig(`--- github.com: - user: keiyuri - oauth_token: 123456` - - var root yaml.Node - err := yaml.Unmarshal([]byte(oldStyle), &root) - if err != nil { - panic("failed to parse test yaml") - } - - buf := bytes.NewBufferString("") - defer StubWriteConfig(buf, nil)() + oauth_token: 123456 +`, "")() + mainBuf := bytes.Buffer{} + hostsBuf := bytes.Buffer{} + defer StubWriteConfig(&mainBuf, &hostsBuf)() defer StubBackupConfig()() - err = migrateConfig("config.yml", &root) + _, err := ParseConfig("config.yml") eq(t, err, nil) - expected := `hosts: - github.com: - oauth_token: "123456" - user: keiyuri + expectedMain := "" + expectedHosts := `github.com: + user: keiyuri + oauth_token: "123456" ` - eq(t, buf.String(), expected) + eq(t, mainBuf.String(), expectedMain) + eq(t, hostsBuf.String(), expectedHosts) } func Test_parseConfigFile(t *testing.T) { diff --git a/internal/config/config_type.go b/internal/config/config_type.go index c2fc66d9b..f965b2c66 100644 --- a/internal/config/config_type.go +++ b/internal/config/config_type.go @@ -54,6 +54,7 @@ func (cm *ConfigMap) SetStringValue(key, value string) error { } valueNode = &yaml.Node{ Kind: yaml.ScalarNode, + Tag: "!!str", Value: "", } @@ -168,16 +169,42 @@ func (c *fileConfig) configForHost(hostname string) (*HostConfig, error) { } func (c *fileConfig) Write() error { - marshalled, err := yaml.Marshal(c.documentRoot) + mainData := yaml.Node{Kind: yaml.MappingNode} + hostsData := yaml.Node{Kind: yaml.MappingNode} + + nodes := c.documentRoot.Content[0].Content + for i := 0; i < len(nodes)-1; i += 2 { + if nodes[i].Value == "hosts" { + hostsData.Content = append(hostsData.Content, nodes[i+1].Content...) + } else { + mainData.Content = append(mainData.Content, nodes[i], nodes[i+1]) + } + } + + mainBytes, err := yaml.Marshal(&mainData) if err != nil { return err } - if bytes.Equal(marshalled, []byte("{}\n")) { - marshalled = []byte{} + fn := ConfigFile() + err = WriteConfigFile(fn, yamlNormalize(mainBytes)) + if err != nil { + return err } - return WriteConfigFile(ConfigFile(), marshalled) + hostsBytes, err := yaml.Marshal(&hostsData) + if err != nil { + return err + } + + return WriteConfigFile(hostsConfigFile(fn), yamlNormalize(hostsBytes)) +} + +func yamlNormalize(b []byte) []byte { + if bytes.Equal(b, []byte("{}\n")) { + return []byte{} + } + return b } func (c *fileConfig) hostEntries() ([]*HostConfig, error) { diff --git a/internal/config/config_type_test.go b/internal/config/config_type_test.go index 97ffd9aa7..1c8105186 100644 --- a/internal/config/config_type_test.go +++ b/internal/config/config_type_test.go @@ -8,8 +8,9 @@ import ( ) func Test_fileConfig_Set(t *testing.T) { - cb := bytes.Buffer{} - StubWriteConfig(&cb, nil) + mainBuf := bytes.Buffer{} + hostsBuf := bytes.Buffer{} + defer StubWriteConfig(&mainBuf, &hostsBuf)() c := NewBlankConfig() assert.NoError(t, c.Set("", "editor", "nano")) @@ -18,22 +19,23 @@ func Test_fileConfig_Set(t *testing.T) { assert.NoError(t, c.Set("github.com", "user", "hubot")) assert.NoError(t, c.Write()) - assert.Equal(t, `editor: nano -hosts: - github.com: - git_protocol: ssh - user: hubot - example.com: - editor: vim -`, cb.String()) + assert.Equal(t, "editor: nano\n", mainBuf.String()) + assert.Equal(t, `github.com: + git_protocol: ssh + user: hubot +example.com: + editor: vim +`, hostsBuf.String()) } func Test_fileConfig_Write(t *testing.T) { - cb := bytes.Buffer{} - StubWriteConfig(&cb, nil) + mainBuf := bytes.Buffer{} + hostsBuf := bytes.Buffer{} + defer StubWriteConfig(&mainBuf, &hostsBuf)() c := NewBlankConfig() assert.NoError(t, c.Write()) - assert.Equal(t, "", cb.String()) + assert.Equal(t, "", mainBuf.String()) + assert.Equal(t, "", hostsBuf.String()) } From 8e70fe939dc7b07b11cb3a117460efb1d6cb26d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 2 Jun 2020 15:39:56 +0200 Subject: [PATCH 09/14] :fire: unused constant --- internal/config/config_type.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/config/config_type.go b/internal/config/config_type.go index f965b2c66..e8953c75c 100644 --- a/internal/config/config_type.go +++ b/internal/config/config_type.go @@ -8,7 +8,6 @@ import ( "gopkg.in/yaml.v3" ) -const defaultHostname = "github.com" const defaultGitProtocol = "https" // This interface describes interacting with some persistent configuration for gh. From c1a518ef8e8ced9a679525747dbe029fad415e52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 3 Jun 2020 13:43:54 +0200 Subject: [PATCH 10/14] Un-export AuthFlow --- internal/config/config_setup.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/config/config_setup.go b/internal/config/config_setup.go index 5bb5f694f..7217eb3d9 100644 --- a/internal/config/config_setup.go +++ b/internal/config/config_setup.go @@ -26,7 +26,7 @@ func IsGitHubApp(id string) bool { } func AuthFlowWithConfig(cfg Config, hostname, notice string) (string, error) { - token, userLogin, err := AuthFlow(hostname, notice) + token, userLogin, err := authFlow(hostname, notice) if err != nil { return "", err } @@ -49,7 +49,7 @@ func AuthFlowWithConfig(cfg Config, hostname, notice string) (string, error) { return token, nil } -func AuthFlow(oauthHost, notice string) (string, string, error) { +func authFlow(oauthHost, notice string) (string, string, error) { var verboseStream io.Writer if strings.Contains(os.Getenv("DEBUG"), "oauth") { verboseStream = os.Stderr From bee132300ca100d2298cf7fd98ef4d4089fb6d7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 3 Jun 2020 13:44:04 +0200 Subject: [PATCH 11/14] Fix overriding OAuth client ID & secret at build time --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index dd863cb0d..b925c11fd 100644 --- a/Makefile +++ b/Makefile @@ -11,8 +11,8 @@ endif LDFLAGS := -X github.com/cli/cli/command.Version=$(GH_VERSION) $(LDFLAGS) LDFLAGS := -X github.com/cli/cli/command.BuildDate=$(BUILD_DATE) $(LDFLAGS) ifdef GH_OAUTH_CLIENT_SECRET - LDFLAGS := -X github.com/cli/cli/context.oauthClientID=$(GH_OAUTH_CLIENT_ID) $(LDFLAGS) - LDFLAGS := -X github.com/cli/cli/context.oauthClientSecret=$(GH_OAUTH_CLIENT_SECRET) $(LDFLAGS) + LDFLAGS := -X github.com/cli/cli/internal/config.oauthClientID=$(GH_OAUTH_CLIENT_ID) $(LDFLAGS) + LDFLAGS := -X github.com/cli/cli/internal/config.oauthClientSecret=$(GH_OAUTH_CLIENT_SECRET) $(LDFLAGS) endif bin/gh: $(BUILD_FILES) From 5b872e7397b65bec63c594648a62dec59e2b1e25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 3 Jun 2020 13:50:18 +0200 Subject: [PATCH 12/14] Rename `fn` to `filename` across the config package --- internal/config/config_file.go | 38 +++++++++++++++++----------------- internal/config/config_type.go | 6 +++--- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/internal/config/config_file.go b/internal/config/config_file.go index 71fffb0d1..779980c72 100644 --- a/internal/config/config_file.go +++ b/internal/config/config_file.go @@ -21,16 +21,16 @@ func ConfigFile() string { return path.Join(ConfigDir(), "config.yml") } -func hostsConfigFile(fn string) string { - return path.Join(path.Dir(fn), "hosts.yml") +func hostsConfigFile(filename string) string { + return path.Join(path.Dir(filename), "hosts.yml") } func ParseDefaultConfig() (Config, error) { return ParseConfig(ConfigFile()) } -var ReadConfigFile = func(fn string) ([]byte, error) { - f, err := os.Open(fn) +var ReadConfigFile = func(filename string) ([]byte, error) { + f, err := os.Open(filename) if err != nil { return nil, err } @@ -44,13 +44,13 @@ var ReadConfigFile = func(fn string) ([]byte, error) { return data, nil } -var WriteConfigFile = func(fn string, data []byte) error { - err := os.MkdirAll(path.Dir(fn), 0771) +var WriteConfigFile = func(filename string, data []byte) error { + err := os.MkdirAll(path.Dir(filename), 0771) if err != nil { return err } - cfgFile, err := os.OpenFile(fn, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0600) // cargo coded from setup + cfgFile, err := os.OpenFile(filename, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0600) // cargo coded from setup if err != nil { return err } @@ -64,12 +64,12 @@ var WriteConfigFile = func(fn string, data []byte) error { return err } -var BackupConfigFile = func(fn string) error { - return os.Rename(fn, fn+".bak") +var BackupConfigFile = func(filename string) error { + return os.Rename(filename, filename+".bak") } -func parseConfigFile(fn string) ([]byte, *yaml.Node, error) { - data, err := ReadConfigFile(fn) +func parseConfigFile(filename string) ([]byte, *yaml.Node, error) { + data, err := ReadConfigFile(filename) if err != nil { return nil, nil, err } @@ -102,8 +102,8 @@ func isLegacy(root *yaml.Node) bool { return false } -func migrateConfig(fn string) error { - b, err := ReadConfigFile(fn) +func migrateConfig(filename string) error { + b, err := ReadConfigFile(filename) if err != nil { return err } @@ -126,7 +126,7 @@ func migrateConfig(fn string) error { } } - err = BackupConfigFile(fn) + err = BackupConfigFile(filename) if err != nil { return fmt.Errorf("failed to back up existing config: %w", err) } @@ -134,24 +134,24 @@ func migrateConfig(fn string) error { return cfg.Write() } -func ParseConfig(fn string) (Config, error) { - _, root, err := parseConfigFile(fn) +func ParseConfig(filename string) (Config, error) { + _, root, err := parseConfigFile(filename) if err != nil { return nil, err } if isLegacy(root) { - err = migrateConfig(fn) + err = migrateConfig(filename) if err != nil { return nil, fmt.Errorf("error migrating legacy config: %w", err) } - _, root, err = parseConfigFile(fn) + _, root, err = parseConfigFile(filename) if err != nil { return nil, fmt.Errorf("failed to reparse migrated config: %w", err) } } else { - if _, hostsRoot, err := parseConfigFile(hostsConfigFile(fn)); err == nil { + if _, hostsRoot, err := parseConfigFile(hostsConfigFile(filename)); err == nil { if len(hostsRoot.Content[0].Content) > 0 { newContent := []*yaml.Node{ {Value: "hosts"}, diff --git a/internal/config/config_type.go b/internal/config/config_type.go index e8953c75c..e094f3b33 100644 --- a/internal/config/config_type.go +++ b/internal/config/config_type.go @@ -185,8 +185,8 @@ func (c *fileConfig) Write() error { return err } - fn := ConfigFile() - err = WriteConfigFile(fn, yamlNormalize(mainBytes)) + filename := ConfigFile() + err = WriteConfigFile(filename, yamlNormalize(mainBytes)) if err != nil { return err } @@ -196,7 +196,7 @@ func (c *fileConfig) Write() error { return err } - return WriteConfigFile(hostsConfigFile(fn), yamlNormalize(hostsBytes)) + return WriteConfigFile(hostsConfigFile(filename), yamlNormalize(hostsBytes)) } func yamlNormalize(b []byte) []byte { From b48237aa598164d8de3b738cbc8ca9075940d5ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 3 Jun 2020 15:16:55 +0200 Subject: [PATCH 13/14] Update headless authentication instructions --- auth/oauth.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/auth/oauth.go b/auth/oauth.go index 5e286443e..3568956c5 100644 --- a/auth/oauth.go +++ b/auth/oauth.go @@ -66,9 +66,9 @@ func (oa *OAuthFlow) ObtainAccessToken() (accessToken string, err error) { fmt.Fprintf(os.Stderr, "Please open the following URL manually:\n%s\n", startURL) fmt.Fprintf(os.Stderr, "") // TODO: Temporary workaround for https://github.com/cli/cli/issues/297 - fmt.Fprintf(os.Stderr, "If you are on a server or other headless system, use this workaround instead:") - fmt.Fprintf(os.Stderr, " 1. Complete authentication on a GUI system") - fmt.Fprintf(os.Stderr, " 2. Copy the contents of ~/.config/gh/config.yml to this system") + fmt.Fprintf(os.Stderr, "If you are on a server or other headless system, use this workaround instead:\n") + fmt.Fprintf(os.Stderr, " 1. Complete authentication on a GUI system;\n") + fmt.Fprintf(os.Stderr, " 2. Copy the contents of `~/.config/gh/hosts.yml` to this system.\n") } _ = http.Serve(listener, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From 9a5b628001049d4976ce3d77b63816397db3bfad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 4 Jun 2020 12:22:26 +0200 Subject: [PATCH 14/14] Ensure consistent order of yaml keys in `migrateConfig` --- internal/config/config_file.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/config/config_file.go b/internal/config/config_file.go index 779980c72..49a2770d3 100644 --- a/internal/config/config_file.go +++ b/internal/config/config_file.go @@ -108,7 +108,7 @@ func migrateConfig(filename string) error { return err } - var hosts map[string][]map[string]string + var hosts map[string][]yaml.Node err = yaml.Unmarshal(b, &hosts) if err != nil { return fmt.Errorf("error decoding legacy format: %w", err) @@ -119,8 +119,9 @@ func migrateConfig(filename string) error { if len(entries) < 1 { continue } - for key, value := range entries[0] { - if err := cfg.Set(hostname, key, value); err != nil { + mapContent := entries[0].Content + for i := 0; i < len(mapContent)-1; i += 2 { + if err := cfg.Set(hostname, mapContent[i].Value, mapContent[i+1].Value); err != nil { return err } }