diff --git a/internal/ghcmd/cmd.go b/internal/ghcmd/cmd.go index eab842c5a..7350437df 100644 --- a/internal/ghcmd/cmd.go +++ b/internal/ghcmd/cmd.go @@ -52,13 +52,18 @@ func Main() exitCode { buildVersion := build.Version hasDebug, _ := utils.IsDebugEnabled() - cfg, err := config.NewConfig() - if err != nil { - fmt.Fprintf(os.Stderr, "failed to load config: %s\n", err) - return exitError + cfg, cfgErr := config.NewConfig() + if cfgErr != nil { + fmt.Fprintf(os.Stderr, "warning: failed to load config: %s\n", cfgErr) } + cfgFunc := func() (gh.Config, error) { return cfg, cfgErr } - ioStreams := newIOStreams(cfg) + var ioStreams *iostreams.IOStreams + if cfgErr == nil { + ioStreams = newIOStreams(cfg) + } else { + ioStreams = iostreams.System() + } stderr := ioStreams.ErrOut ghExecutablePath := executablePath("gh") @@ -70,9 +75,13 @@ func Main() exitCode { } var telemetryService ghtelemetry.Service - if os.Getenv("GH_PRIVATE_ENABLE_TELEMETRY") == "" || mightBeGHESUser(cfg) { + switch { + case cfgErr != nil: + // Without a valid on-disk config we can't honour user telemetry preferences, so disable it to be safe. telemetryService = &telemetry.NoOpService{} - } else { + case os.Getenv("GH_PRIVATE_ENABLE_TELEMETRY") == "" || mightBeGHESUser(cfg): + telemetryService = &telemetry.NoOpService{} + default: telemetryState := telemetry.ParseTelemetryState(cfg.Telemetry().Value) switch telemetryState { case telemetry.Disabled: @@ -100,12 +109,14 @@ func Main() exitCode { } defer telemetryService.Flush() - cmdFactory := factory.New(buildVersion, string(agents.Detect()), cfg, ioStreams, ghExecutablePath, telemetryService) + cmdFactory := factory.New(buildVersion, string(agents.Detect()), cfgFunc, ioStreams, ghExecutablePath, telemetryService) - var m migration.MultiAccount - if err := cfg.Migrate(m); err != nil { - fmt.Fprintln(stderr, err) - return exitError + if cfgErr == nil { + var m migration.MultiAccount + if err := cfg.Migrate(m); err != nil { + fmt.Fprintln(stderr, err) + return exitError + } } ctx := context.Background() diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index b046ec77d..67fb8b762 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -354,6 +354,13 @@ func SpawnSendTelemetry(executable string, payload SendTelemetryPayload) { return } + // Resolve the executable to an absolute path before changing the child's + // working directory. Without this, a relative path (e.g. from GH_PATH) would + // be resolved against cmd.Dir at Start time and fail to spawn. + if abs, err := filepath.Abs(executable); err == nil { + executable = abs + } + cmd := exec.Command(executable, "send-telemetry") cmd.Stdout = io.Discard @@ -362,7 +369,10 @@ func SpawnSendTelemetry(executable string, payload SendTelemetryPayload) { // Set the working directory to a stable directory elsewhere so that the subprocess doesn't // hold a reference to the parent's current working directory, avoiding any weirdness around // deleting the parent process's current working directory while the child is still running. - cmd.Dir = os.TempDir() + // Only do this when we have an absolute executable path so that the child can still be found. + if filepath.IsAbs(executable) { + cmd.Dir = os.TempDir() + } // Configure the child process to be detached from the parent so that it can continue running // after the parent exits, and so that it doesn't receive any signals sent to the parent. @@ -381,7 +391,8 @@ func SpawnSendTelemetry(executable string, payload SendTelemetryPayload) { // Write the payload synchronously into the kernel pipe buffer, then close // the pipe to signal EOF. The child reads the complete payload from stdin. - _, _ = stdin.Write(payloadBytes) + // io.Copy loops until all bytes are written, avoiding any risk of a short write. + _, _ = io.Copy(stdin, bytes.NewReader(payloadBytes)) _ = stdin.Close() // Release resources associated with the child process since we will never Wait for it. diff --git a/pkg/cmd/attestation/verify/verify_integration_test.go b/pkg/cmd/attestation/verify/verify_integration_test.go index 10a1e5216..b99941413 100644 --- a/pkg/cmd/attestation/verify/verify_integration_test.go +++ b/pkg/cmd/attestation/verify/verify_integration_test.go @@ -30,7 +30,7 @@ func TestVerifyIntegration(t *testing.T) { ios, _, _, _ := iostreams.Test() hc, err := factory.HttpClientFunc( - &config.AuthConfig{}, + func() (gh.Config, error) { return config.NewBlankConfig(), nil }, ios, "test", "", @@ -150,7 +150,7 @@ func TestVerifyIntegrationCustomIssuer(t *testing.T) { ios, _, _, _ := iostreams.Test() hc, err := factory.HttpClientFunc( - &config.AuthConfig{}, + func() (gh.Config, error) { return config.NewBlankConfig(), nil }, ios, "test", "", @@ -228,7 +228,7 @@ func TestVerifyIntegrationReusableWorkflow(t *testing.T) { cfg := config.NewBlankConfig() ios, _, _, _ := iostreams.Test() hc, err := factory.HttpClientFunc( - cfg.Authentication(), + func() (gh.Config, error) { return cfg, nil }, ios, "test", "", @@ -325,7 +325,7 @@ func TestVerifyIntegrationReusableWorkflowSignerWorkflow(t *testing.T) { cfg := config.NewBlankConfig() ios, _, _, _ := iostreams.Test() hc, err := factory.HttpClientFunc( - cfg.Authentication(), + func() (gh.Config, error) { return cfg, nil }, ios, "test", "", diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index bf203bd43..f61e51b45 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -23,19 +23,16 @@ import ( var ssoHeader string var ssoURLRE = regexp.MustCompile(`\burl=([^;]+)`) -func New(appVersion string, invokingAgent string, cfg gh.Config, ios *iostreams.IOStreams, executablePath string, telemetryDisabler ghtelemetry.Disabler) *cmdutil.Factory { +func New(appVersion string, invokingAgent string, cfgFunc func() (gh.Config, error), ios *iostreams.IOStreams, executablePath string, telemetryDisabler ghtelemetry.Disabler) *cmdutil.Factory { f := &cmdutil.Factory{ - AppVersion: appVersion, - InvokingAgent: invokingAgent, - Cfg: cfg, - Config: func() (gh.Config, error) { - return cfg, nil - }, // No factory dependencies + AppVersion: appVersion, + InvokingAgent: invokingAgent, + Config: cfgFunc, ExecutablePath: executablePath, } f.IOStreams = ios - f.HttpClient = HttpClientFunc(cfg.Authentication(), ios, appVersion, invokingAgent, telemetryDisabler) + f.HttpClient = HttpClientFunc(cfgFunc, ios, appVersion, invokingAgent, telemetryDisabler) f.PlainHttpClient = plainHttpClientFunc(ios, appVersion, invokingAgent, telemetryDisabler) f.GitClient = newGitClient(f) // Depends on IOStreams, and Executable f.Remotes = remotesFunc(f) // Depends on Config, and GitClient @@ -187,10 +184,14 @@ func remotesFunc(f *cmdutil.Factory) func() (ghContext.Remotes, error) { return rr.Resolver() } -func HttpClientFunc(authCfg gh.AuthConfig, ios *iostreams.IOStreams, appVersion string, invokingAgent string, telemetryDisabler ghtelemetry.Disabler) func() (*http.Client, error) { +func HttpClientFunc(cfgFunc func() (gh.Config, error), ios *iostreams.IOStreams, appVersion string, invokingAgent string, telemetryDisabler ghtelemetry.Disabler) func() (*http.Client, error) { return func() (*http.Client, error) { + cfg, err := cfgFunc() + if err != nil { + return nil, err + } opts := api.HTTPClientOptions{ - Config: authCfg, + Config: cfg.Authentication(), Log: ios.ErrOut, LogColorize: ios.ColorEnabled(), AppVersion: appVersion, diff --git a/pkg/cmd/factory/default_test.go b/pkg/cmd/factory/default_test.go index 6f376e48c..9cf34f3b0 100644 --- a/pkg/cmd/factory/default_test.go +++ b/pkg/cmd/factory/default_test.go @@ -353,7 +353,7 @@ func TestSSOURL(t *testing.T) { t.Run(tt.name, func(t *testing.T) { cfg := config.NewBlankConfig() ios, _, _, stderr := iostreams.Test() - client, err := HttpClientFunc(cfg.Authentication(), ios, "v1.2.3", "", &telemetry.NoOpService{})() + client, err := HttpClientFunc(func() (gh.Config, error) { return cfg, nil }, ios, "v1.2.3", "", &telemetry.NoOpService{})() require.NoError(t, err) req, err := http.NewRequest("GET", ts.URL, nil) if tt.sso != "" { diff --git a/pkg/cmd/root/help_topic.go b/pkg/cmd/root/help_topic.go index fbacef356..3002a3512 100644 --- a/pkg/cmd/root/help_topic.go +++ b/pkg/cmd/root/help_topic.go @@ -118,10 +118,10 @@ var HelpTopics = []helpTopic{ more compatible with speech synthesis and braille screen readers. %[1]sGH_TELEMETRY%[1]s: set to %[1]slog%[1]s to print telemetry data to standard error instead of sending it. - Set to %[1]sfalse%[1]s or %[1]s0%[1]s to disable telemetry that would have been printed when set to %[1]slog%[1]s. - - %[1]sDO_NOT_TRACK%[1]s: set to %[1]strue%[1]s or %[1]s1%[1]s to disable telemetry that would have been printed - when %[1]sGH_TELEMETRY%[1]s is set to %[1]slog%[1]s. %[1]sGH_TELEMETRY%[1]s takes precedence if both are set. + Set to %[1]sfalse%[1]s or %[1]s0%[1]s to disable telemetry. Takes precedence over %[1]sDO_NOT_TRACK%[1]s. + + %[1]sDO_NOT_TRACK%[1]s: set to %[1]strue%[1]s or %[1]s1%[1]s to disable telemetry. Ignored when + %[1]sGH_TELEMETRY%[1]s is set, which takes precedence. %[1]sGH_SPINNER_DISABLED%[1]s: set to a truthy value to replace the spinner animation with a textual progress indicator. diff --git a/pkg/cmdutil/factory.go b/pkg/cmdutil/factory.go index 9ea8ce4a6..1faf859f0 100644 --- a/pkg/cmdutil/factory.go +++ b/pkg/cmdutil/factory.go @@ -26,9 +26,13 @@ type Factory struct { BaseRepo func() (ghrepo.Interface, error) Branch func() (string, error) - Cfg gh.Config - // TODO: Config should be removed in favour of cfg being passed to the right place, - // but this is going to be very invasive and shouldn't be done as part of a feature change. + // It would be nice if Config were just loaded once at startup and an error + // were returned, but this would prevent commands like "gh version" from running. + // So for now, we eagerly load the config and don't fail if there is an error, + // and defer the error handling to commands that need it. + // HOWEVER, as an additional point, the root command setup currently DOES call + // this and errors, so we never get to "gh version" anyway. + // We need to revisit that, but I don't want to make it worse. Config func() (gh.Config, error) HttpClient func() (*http.Client, error) // PlainHttpClient is a special HTTP client that does not automatically set