Merge pull request cli#3992 from despreston/858-config-browser

samcoe · web-flow · commit 8fb6bb66c8af · 2021-08-17T14:17:54.000-07:00
add browser option to config
diff --git a/internal/config/config_type.go b/internal/config/config_type.go
@@ -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 web browser to use for opening URLs",
+		DefaultValue: "",
+	},
 }
 
 func ConfigOptions() []ConfigOption {
@@ -193,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
@@ -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
@@ -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
 }
@@ -91,7 +91,26 @@ func httpClientFunc(f *cmdutil.Factory, appVersion string) func() (*http.Client,
 
 func browser(f *cmdutil.Factory) cmdutil.Browser {
 	io := f.IOStreams
-	return cmdutil.NewBrowser(os.Getenv("BROWSER"), io.Out, io.ErrOut)
+	return cmdutil.NewBrowser(browserLauncher(f), io.Out, io.ErrOut)
+}
+
+// Browser precedence
+// 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 cfgBrowser, _ := cfg.Get("", "browser"); cfgBrowser != "" {
+			return cfgBrowser
+		}
+	}
+
+	return os.Getenv("BROWSER")
 }
 
 func executable() string {
diff --git a/pkg/cmd/factory/default_test.go b/pkg/cmd/factory/default_test.go
@@ -378,6 +378,80 @@ 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: "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{
+				"BROWSER": "BROWSER",
+			},
+			wantBrowser: "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:      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) {
+			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:
diff --git 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.
