From c95f30af800ecae9be99e1348044e277c33e0e37 Mon Sep 17 00:00:00 2001 From: Des Preston Date: Tue, 13 Jul 2021 15:06:40 -0400 Subject: [PATCH 1/4] add browser option to config Allows setting the path to the browser using the config. Closes #858 --- internal/config/config_type.go | 9 +++++++-- pkg/cmd/factory/default.go | 13 +++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/internal/config/config_type.go b/internal/config/config_type.go index 3714698bd..61bda9847 100644 --- a/internal/config/config_type.go +++ b/internal/config/config_type.go @@ -8,8 +8,8 @@ import ( // This interface describes interacting with some persistent configuration for gh. type Config interface { - Get(string, string) (string, error) - GetWithSource(string, string) (string, string, error) + Get(hostname string, key string) (string, error) + GetWithSource(hostname string, key string) (string, string, error) Set(string, string, string) error UnsetHost(string) Hosts() ([]string, error) @@ -55,6 +55,11 @@ var configOptions = []ConfigOption{ Description: "the path to a unix socket through which to make HTTP connection", DefaultValue: "", }, + { + Key: "browser", + Description: "the path to the browser to use when opening URLs", + DefaultValue: "", + }, } func ConfigOptions() []ConfigOption { diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 32187689b..95eca69a8 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -89,8 +89,21 @@ func httpClientFunc(f *cmdutil.Factory, appVersion string) func() (*http.Client, } } +// browser returns the path to the browser. Attempts to read a path from Config, +// and falls back to the $BROWSER env var if Config value is blank. func browser(f *cmdutil.Factory) cmdutil.Browser { io := f.IOStreams + + cfg, err := f.Config() + if err != nil { + fmt.Fprintf(io.ErrOut, "problem loading config %s", err) + } + + fromConfig, err := cfg.Get("", "browser") + if err == nil && fromConfig != "" { + return cmdutil.NewBrowser(fromConfig, io.Out, io.ErrOut) + } + return cmdutil.NewBrowser(os.Getenv("BROWSER"), io.Out, io.ErrOut) } From 34b3d5bb8693964134fd8492181f8ae0a4d5c129 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Tue, 17 Aug 2021 10:05:54 -0700 Subject: [PATCH 2/4] Add tests and a little polish --- internal/config/config_type.go | 15 ++++++-- internal/config/config_type_test.go | 9 +++++ pkg/cmd/factory/default.go | 24 ++++++------- pkg/cmd/factory/default_test.go | 55 +++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 15 deletions(-) diff --git a/internal/config/config_type.go b/internal/config/config_type.go index 61bda9847..92792e93f 100644 --- a/internal/config/config_type.go +++ b/internal/config/config_type.go @@ -8,8 +8,8 @@ import ( // This interface describes interacting with some persistent configuration for gh. type Config interface { - Get(hostname string, key string) (string, error) - GetWithSource(hostname string, key string) (string, string, error) + Get(string, string) (string, error) + GetWithSource(string, string) (string, string, error) Set(string, string, string) error UnsetHost(string) Hosts() ([]string, error) @@ -57,7 +57,7 @@ var configOptions = []ConfigOption{ }, { Key: "browser", - Description: "the path to the browser to use when opening URLs", + Description: "the web browser to use for opening URLs", DefaultValue: "", }, } @@ -198,6 +198,15 @@ func NewBlankRoot() *yaml.Node { Kind: yaml.ScalarNode, Value: "", }, + { + HeadComment: "What web browser gh should use when opening URLs. If blank, will refer to environment.", + Kind: yaml.ScalarNode, + Value: "browser", + }, + { + Kind: yaml.ScalarNode, + Value: "", + }, }, }, }, diff --git a/internal/config/config_type_test.go b/internal/config/config_type_test.go index fe5e3f239..bf53aabe4 100644 --- a/internal/config/config_type_test.go +++ b/internal/config/config_type_test.go @@ -52,6 +52,8 @@ func Test_defaultConfig(t *testing.T) { co: pr checkout # The path to a unix socket through which send HTTP connections. If blank, HTTP traffic will be handled by net/http.DefaultTransport. http_unix_socket: + # What web browser gh should use when opening URLs. If blank, will refer to environment. + browser: `) assert.Equal(t, expected, mainBuf.String()) assert.Equal(t, "", hostsBuf.String()) @@ -69,6 +71,10 @@ func Test_defaultConfig(t *testing.T) { assert.Equal(t, len(aliases.All()), 1) expansion, _ := aliases.Get("co") assert.Equal(t, expansion, "pr checkout") + + browser, err := cfg.Get("", "browser") + assert.NoError(t, err) + assert.Equal(t, "", browser) } func Test_ValidateValue(t *testing.T) { @@ -106,4 +112,7 @@ func Test_ValidateKey(t *testing.T) { err = ValidateKey("http_unix_socket") assert.NoError(t, err) + + err = ValidateKey("browser") + assert.NoError(t, err) } diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 95eca69a8..f4cc7b7c2 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -29,7 +29,7 @@ func New(appVersion string) *cmdutil.Factory { f.HttpClient = httpClientFunc(f, appVersion) // Depends on Config, IOStreams, and appVersion f.Remotes = remotesFunc(f) // Depends on Config f.BaseRepo = BaseRepoFunc(f) // Depends on Remotes - f.Browser = browser(f) // Depends on IOStreams + f.Browser = browser(f) // Depends on Config, and IOStreams return f } @@ -89,22 +89,22 @@ func httpClientFunc(f *cmdutil.Factory, appVersion string) func() (*http.Client, } } -// browser returns the path to the browser. Attempts to read a path from Config, -// and falls back to the $BROWSER env var if Config value is blank. func browser(f *cmdutil.Factory) cmdutil.Browser { io := f.IOStreams + return cmdutil.NewBrowser(browserLauncher(f), io.Out, io.ErrOut) +} +// Browser precedence +// 1. browser from config +// 2. BROWSER +func browserLauncher(f *cmdutil.Factory) string { cfg, err := f.Config() - if err != nil { - fmt.Fprintf(io.ErrOut, "problem loading config %s", err) + if err == nil { + if browser, _ := cfg.Get("", "browser"); browser != "" { + return browser + } } - - fromConfig, err := cfg.Get("", "browser") - if err == nil && fromConfig != "" { - return cmdutil.NewBrowser(fromConfig, io.Out, io.ErrOut) - } - - return cmdutil.NewBrowser(os.Getenv("BROWSER"), io.Out, io.ErrOut) + return os.Getenv("BROWSER") } func executable() string { diff --git a/pkg/cmd/factory/default_test.go b/pkg/cmd/factory/default_test.go index c7e641aa1..8935c4a49 100644 --- a/pkg/cmd/factory/default_test.go +++ b/pkg/cmd/factory/default_test.go @@ -378,6 +378,57 @@ func Test_ioStreams_prompt(t *testing.T) { } } +func Test_browserLauncher(t *testing.T) { + tests := []struct { + name string + env map[string]string + config config.Config + wantBrowser string + }{ + { + name: "BROWSER set", + env: map[string]string{ + "BROWSER": "BROWSER", + }, + wantBrowser: "BROWSER", + }, + { + name: "config browser set", + config: browserConfig(), + wantBrowser: "CONFIG_BROWSER", + }, + { + name: "config browser and BROWSER set", + env: map[string]string{ + "BROWSER": "BROWSER", + }, + config: browserConfig(), + wantBrowser: "CONFIG_BROWSER", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.env != nil { + for k, v := range tt.env { + old := os.Getenv(k) + os.Setenv(k, v) + defer os.Setenv(k, old) + } + } + f := New("1") + f.Config = func() (config.Config, error) { + if tt.config == nil { + return config.NewBlankConfig(), nil + } else { + return tt.config, nil + } + } + browser := browserLauncher(f) + assert.Equal(t, tt.wantBrowser, browser) + }) + } +} + func defaultConfig() config.Config { return config.InheritEnv(config.NewFromString(heredoc.Doc(` hosts: @@ -393,3 +444,7 @@ func pagerConfig() config.Config { func disablePromptConfig() config.Config { return config.NewFromString("prompt: disabled") } + +func browserConfig() config.Config { + return config.NewFromString("browser: CONFIG_BROWSER") +} From a07748f1f1ef6ba5cdab497d54e45c171d98dec6 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Tue, 17 Aug 2021 14:07:49 -0700 Subject: [PATCH 3/4] Add support for GH_BROWSER env var --- pkg/cmd/factory/default.go | 14 +++++++++---- pkg/cmd/factory/default_test.go | 35 +++++++++++++++++++++++++-------- 2 files changed, 37 insertions(+), 12 deletions(-) diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index f4cc7b7c2..3693df6d6 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -95,15 +95,21 @@ func browser(f *cmdutil.Factory) cmdutil.Browser { } // Browser precedence -// 1. browser from config -// 2. BROWSER +// 1. GH_BROWSER +// 2. browser from config +// 3. BROWSER func browserLauncher(f *cmdutil.Factory) string { + if ghBrowser := os.Getenv("GH_BROWSER"); ghBrowser != "" { + return ghBrowser + } + cfg, err := f.Config() if err == nil { - if browser, _ := cfg.Get("", "browser"); browser != "" { - return browser + if cfgBrowser, _ := cfg.Get("", "browser"); cfgBrowser != "" { + return cfgBrowser } } + return os.Getenv("BROWSER") } diff --git a/pkg/cmd/factory/default_test.go b/pkg/cmd/factory/default_test.go index 8935c4a49..03ea29153 100644 --- a/pkg/cmd/factory/default_test.go +++ b/pkg/cmd/factory/default_test.go @@ -385,6 +385,18 @@ func Test_browserLauncher(t *testing.T) { config config.Config wantBrowser string }{ + { + name: "GH_BROWSER set", + env: map[string]string{ + "GH_BROWSER": "GH_BROWSER", + }, + wantBrowser: "GH_BROWSER", + }, + { + name: "config browser set", + config: config.NewFromString("browser: CONFIG_BROWSER"), + wantBrowser: "CONFIG_BROWSER", + }, { name: "BROWSER set", env: map[string]string{ @@ -393,18 +405,29 @@ func Test_browserLauncher(t *testing.T) { wantBrowser: "BROWSER", }, { - name: "config browser set", - config: browserConfig(), - wantBrowser: "CONFIG_BROWSER", + name: "GH_BROWSER and config browser set", + env: map[string]string{ + "GH_BROWSER": "GH_BROWSER", + }, + config: config.NewFromString("browser: CONFIG_BROWSER"), + wantBrowser: "GH_BROWSER", }, { name: "config browser and BROWSER set", env: map[string]string{ "BROWSER": "BROWSER", }, - config: browserConfig(), + config: config.NewFromString("browser: CONFIG_BROWSER"), wantBrowser: "CONFIG_BROWSER", }, + { + name: "GH_BROWSER and BROWSER set", + env: map[string]string{ + "BROWSER": "BROWSER", + "GH_BROWSER": "GH_BROWSER", + }, + wantBrowser: "GH_BROWSER", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -444,7 +467,3 @@ func pagerConfig() config.Config { func disablePromptConfig() config.Config { return config.NewFromString("prompt: disabled") } - -func browserConfig() config.Config { - return config.NewFromString("browser: CONFIG_BROWSER") -} From b9438015a214aff7f4ddc9f50c954f06bdfb3492 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Tue, 17 Aug 2021 14:10:15 -0700 Subject: [PATCH 4/4] Add GH_BROWSER to help topic --- pkg/cmd/root/help_topic.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/root/help_topic.go b/pkg/cmd/root/help_topic.go index 040c9fa12..b6a4a158d 100644 --- a/pkg/cmd/root/help_topic.go +++ b/pkg/cmd/root/help_topic.go @@ -44,7 +44,7 @@ var HelpTopics = map[string]map[string]string{ GH_EDITOR, GIT_EDITOR, VISUAL, EDITOR (in order of precedence): the editor tool to use for authoring text. - BROWSER: the web browser to use for opening links. + GH_BROWSER, BROWSER (in order of precedence): the web browser to use for opening links. DEBUG: set to any value to enable verbose output to standard error. Include values "api" or "oauth" to print detailed information about HTTP requests or authentication flow.