From 4a3ae9f9ce6f705dfca03edf1757af0a03e1c7cc Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 23 May 2024 11:15:23 +0200 Subject: [PATCH 01/12] Always display fatalErrors --- cmd/tea_terraform.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/cmd/tea_terraform.go b/cmd/tea_terraform.go index 4c302ba6..3f917240 100644 --- a/cmd/tea_terraform.go +++ b/cmd/tea_terraform.go @@ -111,10 +111,8 @@ func (m cmdModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.fatalErrorSeen = true - // record the fatal error here if it was not from a specific taskModel - if msg.id == 0 { - m.fatalError = msg.err.Error() - } + // record the fatal error here, to repeat it at the end of the process + m.fatalError = msg.err.Error() return m, tea.Sequence( tea.Batch(batch...), From 21dd0b8dacee6c99472ec5e6551198f491d4db81 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 23 May 2024 11:25:45 +0200 Subject: [PATCH 02/12] Rename variable to match pattern --- cmd/tea_terraform.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/cmd/tea_terraform.go b/cmd/tea_terraform.go index 3f917240..72739d99 100644 --- a/cmd/tea_terraform.go +++ b/cmd/tea_terraform.go @@ -90,7 +90,7 @@ func (m cmdModel) Init() tea.Cmd { func (m cmdModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { log.Debugf("cmdModel: Update %T received %#v", msg, msg) - batch := []tea.Cmd{} + cmds := []tea.Cmd{} // special case the messages that need to be handled at this level switch msg := msg.(type) { @@ -115,7 +115,7 @@ func (m cmdModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.fatalError = msg.err.Error() return m, tea.Sequence( - tea.Batch(batch...), + tea.Batch(cmds...), tea.Quit, ) @@ -126,8 +126,8 @@ func (m cmdModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case tokenAvailableMsg: tm, cmd := m.tokenChecks(msg.token) - batch = append(batch, cmd) - return tm, tea.Batch(batch...) + cmds = append(cmds, cmd) + return tm, tea.Batch(cmds...) case runPlanNowMsg, runTfApplyMsg: m.terraformHasStarted = true @@ -137,7 +137,7 @@ func (m cmdModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { skipView(m.View()) case delayQuitMsg: - batch = append(batch, tea.Quit) + cmds = append(cmds, tea.Quit) } @@ -145,7 +145,7 @@ func (m cmdModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { var cmd tea.Cmd m.cmd, cmd = m.cmd.Update(msg) if cmd != nil { - batch = append(batch, cmd) + cmds = append(cmds, cmd) } // pass all messages to all tasks @@ -153,11 +153,11 @@ func (m cmdModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { tm, cmd := t.Update(msg) m.tasks[k] = tm if cmd != nil { - batch = append(batch, cmd) + cmds = append(cmds, cmd) } } - return m, tea.Batch(batch...) + return m, tea.Batch(cmds...) } // skipView scrolls the terminal contents up after ExecCommand() to avoid From ce4230dcf830d503918b4962bb2f138a987a096a Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 23 May 2024 11:30:46 +0200 Subject: [PATCH 03/12] Don't short-circuit fatalError This allows the task emitting the error to show where the error happened while still showing the full error message at the bottom of the screen. --- cmd/tea_initialisesources.go | 13 ++++--------- cmd/tea_taskmodel.go | 6 ++++++ cmd/tea_terraform.go | 9 +++------ 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/cmd/tea_initialisesources.go b/cmd/tea_initialisesources.go index f097e79b..3c8490e4 100644 --- a/cmd/tea_initialisesources.go +++ b/cmd/tea_initialisesources.go @@ -156,17 +156,12 @@ func (m initialiseSourcesModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if msg.id == m.spinner.ID() { m.errors = append(m.errors, fmt.Sprintf("Note: %v", msg.err)) } - case fatalError: - if msg.id == m.spinner.ID() { - m.status = taskStatusError - m.title = markdownToString(fmt.Sprintf("> error while configuring AWS access: %v", msg.err)) - } - default: - var taskCmd tea.Cmd - m.taskModel, taskCmd = m.taskModel.Update(msg) - cmds = append(cmds, taskCmd) } + var taskCmd tea.Cmd + m.taskModel, taskCmd = m.taskModel.Update(msg) + cmds = append(cmds, taskCmd) + // process the form if it is not yet done if m.awsConfigForm != nil && !m.awsConfigFormDone { switch m.awsConfigForm.State { diff --git a/cmd/tea_taskmodel.go b/cmd/tea_taskmodel.go index 27f231c5..06b413c5 100644 --- a/cmd/tea_taskmodel.go +++ b/cmd/tea_taskmodel.go @@ -92,6 +92,12 @@ func (m taskModel) Update(msg tea.Msg) (taskModel, tea.Cmd) { if m.spinner.ID() == msg.id { m.status = msg.status } + case fatalError: + if m.spinner.ID() == msg.id { + m.status = taskStatusError + m.title = fmt.Sprintf("%v: %v", m.title, msg.err) + return m, nil + } default: if m.status == taskStatusRunning { var cmd tea.Cmd diff --git a/cmd/tea_terraform.go b/cmd/tea_terraform.go index 72739d99..38d002aa 100644 --- a/cmd/tea_terraform.go +++ b/cmd/tea_terraform.go @@ -114,10 +114,7 @@ func (m cmdModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // record the fatal error here, to repeat it at the end of the process m.fatalError = msg.err.Error() - return m, tea.Sequence( - tea.Batch(cmds...), - tea.Quit, - ) + cmds = append(cmds, func() tea.Msg { return delayQuitMsg{} }) case instanceLoadedMsg: m.oi = msg.instance @@ -125,9 +122,9 @@ func (m cmdModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // delete(m.tasks, "00_oi") case tokenAvailableMsg: - tm, cmd := m.tokenChecks(msg.token) + var cmd tea.Cmd + m, cmd = m.tokenChecks(msg.token) cmds = append(cmds, cmd) - return tm, tea.Batch(cmds...) case runPlanNowMsg, runTfApplyMsg: m.terraformHasStarted = true From fe19823bb68ff66bcf4b2ca77aad9b2322bc1faf Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 23 May 2024 13:19:21 +0200 Subject: [PATCH 04/12] Remove unused fatalErrorSeen member --- cmd/tea_terraform.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/cmd/tea_terraform.go b/cmd/tea_terraform.go index 38d002aa..7d565a82 100644 --- a/cmd/tea_terraform.go +++ b/cmd/tea_terraform.go @@ -33,7 +33,6 @@ type cmdModel struct { // UI state tasks map[string]tea.Model terraformHasStarted bool // remember whether terraform already has started. this is important to do the correct workarounds on errors. See also `skipView()` - fatalErrorSeen bool // remember whether a fatalError has been seen to avoid showing pending tasks fatalError string // this will get set if there's a fatalError coming through that doesn't have a task ID set // business logic. This model will implement the actual CLI functionality requested. @@ -109,8 +108,6 @@ func (m cmdModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { skipView(m.View()) } - m.fatalErrorSeen = true - // record the fatal error here, to repeat it at the end of the process m.fatalError = msg.err.Error() From 478dc04235bc2ac2cd2865c24e0a18c15d3c4c95 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 23 May 2024 15:02:15 +0200 Subject: [PATCH 05/12] Fix formatting of fatal errors in cmdModel --- cmd/theme.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/theme.go b/cmd/theme.go index 139b51c8..6579978a 100644 --- a/cmd/theme.go +++ b/cmd/theme.go @@ -204,7 +204,7 @@ func MarkdownStyle() ansi.StyleConfig { StylePrimitive: ansi.StylePrimitive{ Italic: ptrBool(true), }, - Indent: ptrUint(2), + Indent: ptrUint(1), IndentToken: ptrString("│ "), }, List: ansi.StyleList{ From 40d23a558a898b97d7847c612b5907b233b49a8c Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 23 May 2024 14:58:31 +0200 Subject: [PATCH 06/12] Fix pre-command views This creates a freeze/unfreezeViewMsg pair that can be used in conjunction with the cliExecCommandModel to render the last View before a command takes over the terminal into the scrollback buffer. This gets rid of the janky skipView() implementation and should be much more reliable as it reuses more of bubbletea's functionality by letting bubbletea clear the View() by returning an empty string when frozen. --- cmd/tea.go | 11 ++++--- cmd/tea_execcommand.go | 66 ++++++++++++++++++++++++++++++++++++++++ cmd/tea_plan.go | 36 +++++++++++++--------- cmd/tea_terraform.go | 69 +++++++++++++++--------------------------- cmd/terraform_apply.go | 34 ++++++++++++--------- cmd/terraform_plan.go | 4 +-- 6 files changed, 140 insertions(+), 80 deletions(-) create mode 100644 cmd/tea_execcommand.go diff --git a/cmd/tea.go b/cmd/tea.go index e4049c04..5a68dc66 100644 --- a/cmd/tea.go +++ b/cmd/tea.go @@ -50,7 +50,7 @@ type FinalReportingModel interface { FinalReport() string } -func CmdWrapper(action string, requiredScopes []string, commandModel func([]string) tea.Model) func(cmd *cobra.Command, args []string) { +func CmdWrapper(action string, requiredScopes []string, commandModel func(args []string, execCommandFunc ExecCommandFunc) tea.Model) func(cmd *cobra.Command, args []string) { return func(cmd *cobra.Command, args []string) { // set up a context for the command ctx, cancel := context.WithCancel(context.Background()) @@ -95,7 +95,7 @@ func CmdWrapper(action string, requiredScopes []string, commandModel func([]stri return err } - p := tea.NewProgram(cmdModel{ + m := cmdModel{ action: action, ctx: ctx, cancel: cancel, @@ -104,14 +104,15 @@ func CmdWrapper(action string, requiredScopes []string, commandModel func([]stri requiredScopes: requiredScopes, apiKey: viper.GetString("api-key"), tasks: map[string]tea.Model{}, - cmd: commandModel(args), - }) + } + m.cmd = commandModel(args, m.NewExecCommand) + p := tea.NewProgram(&m) result, err := p.Run() if err != nil { return fmt.Errorf("could not start program: %w", err) } - cmd, ok := result.(cmdModel) + cmd, ok := result.(*cmdModel) if ok { frm, ok := cmd.cmd.(FinalReportingModel) if ok { diff --git a/cmd/tea_execcommand.go b/cmd/tea_execcommand.go new file mode 100644 index 00000000..e502e671 --- /dev/null +++ b/cmd/tea_execcommand.go @@ -0,0 +1,66 @@ +package cmd + +import ( + "fmt" + "io" + "os/exec" + + tea "github.com/charmbracelet/bubbletea" +) + +type ExecCommandFunc func(cmd *exec.Cmd) tea.ExecCommand + +// NewExecCommand returns a new ExecCommand that will print the last view from +// the parent cmdModel after bubbletea has released the terminal, but before the +// command is run. +func (m *cmdModel) NewExecCommand(c *exec.Cmd) tea.ExecCommand { + return NewExecCommand(m, c) +} + +func NewExecCommand(parent *cmdModel, c *exec.Cmd) *cliExecCommandModel { + return &cliExecCommandModel{ + parent: parent, + Cmd: c, + } +} + +// osExecCommand is a layer over an exec.Cmd that satisfies the ExecCommand +// interface. It prints the last view from +// the parent cmdModel after bubbletea has released the terminal, but before the +// command is run. +type cliExecCommandModel struct { + parent *cmdModel + *exec.Cmd +} + +func (c cliExecCommandModel) Run() error { + _, err := c.Stdout.Write([]byte(c.parent.frozenView)) + if err != nil { + return fmt.Errorf("failed to write view to stdout: %w", err) + } + return c.Cmd.Run() +} + +// SetStdin sets stdin on underlying exec.Cmd to the given io.Reader. +func (c *cliExecCommandModel) SetStdin(r io.Reader) { + // If unset, have the command use the same input as the terminal. + if c.Stdin == nil { + c.Stdin = r + } +} + +// SetStdout sets stdout on underlying exec.Cmd to the given io.Writer. +func (c *cliExecCommandModel) SetStdout(w io.Writer) { + // If unset, have the command use the same output as the terminal. + if c.Stdout == nil { + c.Stdout = w + } +} + +// SetStderr sets stderr on the underlying exec.Cmd to the given io.Writer. +func (c *cliExecCommandModel) SetStderr(w io.Writer) { + // If unset, use stderr for the command's stderr + if c.Stderr == nil { + c.Stderr = w + } +} diff --git a/cmd/tea_plan.go b/cmd/tea_plan.go index 3279ab11..7c2df4ae 100644 --- a/cmd/tea_plan.go +++ b/cmd/tea_plan.go @@ -22,19 +22,21 @@ type runPlanModel struct { args []string planFile string - revlinkTask revlinkWarmupModel + execCommandFunc ExecCommandFunc + revlinkTask revlinkWarmupModel taskModel } type runPlanNowMsg struct{} type runPlanFinishedMsg struct{} -func NewRunPlanModel(args []string, planFile string) runPlanModel { +func NewRunPlanModel(args []string, planFile string, execCommandFunc ExecCommandFunc) runPlanModel { return runPlanModel{ args: args, planFile: planFile, - revlinkTask: NewRevlinkWarmupModel(), - taskModel: NewTaskModel("Planning Changes"), + revlinkTask: NewRevlinkWarmupModel(), + execCommandFunc: execCommandFunc, + taskModel: NewTaskModel("Planning Changes"), } } @@ -77,22 +79,26 @@ func (m runPlanModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { c = exec.CommandContext(m.ctx, "bash", "-c", "for i in $(seq 100); do echo fake terraform plan progress line $i of 100; done; sleep 1") } - _, span := tracing.Tracer().Start(m.ctx, "terraform plan", trace.WithAttributes( + _, span := tracing.Tracer().Start(m.ctx, "terraform plan", trace.WithAttributes( // nolint:spancheck // will be ended in the tea.Exec cleanup func attribute.String("command", strings.Join(m.args, " ")), )) - cmds = append(cmds, tea.ExecProcess( - c, - func(err error) tea.Msg { - defer span.End() - - if err != nil { - return fatalError{err: fmt.Errorf("failed to run terraform plan: %w", err)} - } - return runPlanFinishedMsg{} - })) + cmds = append(cmds, + tea.Sequence( + func() tea.Msg { return freezeViewMsg{} }, + tea.Exec( // nolint:spancheck // will be ended in the tea.Exec cleanup func + m.execCommandFunc(c), + func(err error) tea.Msg { + defer span.End() + + if err != nil { + return fatalError{err: fmt.Errorf("failed to run terraform plan: %w", err)} + } + return runPlanFinishedMsg{} + }))) case runPlanFinishedMsg: m.taskModel.status = taskStatusDone + cmds = append(cmds, func() tea.Msg { return unfreezeViewMsg{} }) default: // var cmd tea.Cmd diff --git a/cmd/tea_terraform.go b/cmd/tea_terraform.go index 7d565a82..6ab38858 100644 --- a/cmd/tea_terraform.go +++ b/cmd/tea_terraform.go @@ -31,14 +31,19 @@ type cmdModel struct { requiredScopes []string // UI state - tasks map[string]tea.Model - terraformHasStarted bool // remember whether terraform already has started. this is important to do the correct workarounds on errors. See also `skipView()` - fatalError string // this will get set if there's a fatalError coming through that doesn't have a task ID set + tasks map[string]tea.Model + fatalError string // this will get set if there's a fatalError coming through that doesn't have a task ID set + + frozen bool + frozenView string // this gets set if the view is frozen, and will be used to render the last view using the cliExecCommand // business logic. This model will implement the actual CLI functionality requested. cmd tea.Model } +type freezeViewMsg struct{} +type unfreezeViewMsg struct{} + type delayQuitMsg struct{} // fatalError is a wrapper for errors that should abort the running tea.Program. @@ -53,7 +58,7 @@ type otherError struct { err error } -func (m cmdModel) Init() tea.Cmd { +func (m *cmdModel) Init() tea.Cmd { // use the main cli context to not take this time from the main timeout m.tasks["00_oi"] = NewInstanceLoaderModel(m.ctx, m.app) m.tasks["01_token"] = NewEnsureTokenModel(m.ctx, m.app, m.apiKey, m.requiredScopes) @@ -86,7 +91,7 @@ func (m cmdModel) Init() tea.Cmd { ) } -func (m cmdModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { +func (m *cmdModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { log.Debugf("cmdModel: Update %T received %#v", msg, msg) cmds := []tea.Cmd{} @@ -97,17 +102,16 @@ func (m cmdModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if msg.String() == "ctrl+c" { return m, tea.Quit } + case freezeViewMsg: + m.frozenView = m.View() + m.frozen = true + case unfreezeViewMsg: + m.frozen = false + m.frozenView = "" case fatalError: log.WithError(msg.err).WithField("msg.id", msg.id).Debug("cmdModel: fatalError received") - // skipView based on the previous view. While this is not perfect, it's - // the best we can currently do without taking complete control of - // terraform command i/o - if m.terraformHasStarted { - skipView(m.View()) - } - // record the fatal error here, to repeat it at the end of the process m.fatalError = msg.err.Error() @@ -120,16 +124,9 @@ func (m cmdModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case tokenAvailableMsg: var cmd tea.Cmd - m, cmd = m.tokenChecks(msg.token) + cmd = m.tokenChecks(msg.token) cmds = append(cmds, cmd) - case runPlanNowMsg, runTfApplyMsg: - m.terraformHasStarted = true - - case runPlanFinishedMsg, tfApplyFinishedMsg: - // bump screen after terraform ran - skipView(m.View()) - case delayQuitMsg: cmds = append(cmds, tea.Quit) @@ -154,28 +151,9 @@ func (m cmdModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, tea.Batch(cmds...) } -// skipView scrolls the terminal contents up after ExecCommand() to avoid -// overwriting the output from terraform when rendering the next View(). this -// has to be used here in the cmdModel to catch the entire View() output. -// -// NOTE: this is quite brittle and _requires_ that the View() after terraform -// returned is at least many lines as the view before ExecCommand(), otherwise -// the difference will get eaten by bubbletea on re-rendering. -// -// TODO: make this hack less ugly -func skipView(view string) { - lines := strings.Split(view, "\n") - for range lines { - fmt.Println() - } - - // log.Debugf("printed %v lines:", len(lines)) - // log.Debug(lines) -} - -func (m cmdModel) tokenChecks(token *oauth2.Token) (cmdModel, tea.Cmd) { +func (m *cmdModel) tokenChecks(token *oauth2.Token) tea.Cmd { if viper.GetString("ovm-test-fake") != "" { - return m, func() tea.Msg { + return func() tea.Msg { return loadSourcesConfigMsg{ ctx: m.ctx, oi: m.oi, @@ -189,10 +167,10 @@ func (m cmdModel) tokenChecks(token *oauth2.Token) (cmdModel, tea.Cmd) { // permission auth0 will just not assign those scopes rather than fail ok, missing, err := HasScopesFlexible(token, m.requiredScopes) if err != nil { - return m, func() tea.Msg { return fatalError{err: fmt.Errorf("error checking token scopes: %w", err)} } + return func() tea.Msg { return fatalError{err: fmt.Errorf("error checking token scopes: %w", err)} } } if !ok { - return m, func() tea.Msg { + return func() tea.Msg { return fatalError{err: fmt.Errorf("authenticated successfully, but you don't have the required permission: '%v'", missing)} } } @@ -210,7 +188,7 @@ func (m cmdModel) tokenChecks(token *oauth2.Token) (cmdModel, tea.Cmd) { // for now, and we still need a good idea for a better way. Especially as // some of the models require access to viper (for GetConfig/SetConfig) or // contortions to store that data somewhere else. - return m, func() tea.Msg { + return func() tea.Msg { return loadSourcesConfigMsg{ ctx: m.ctx, oi: m.oi, @@ -221,6 +199,9 @@ func (m cmdModel) tokenChecks(token *oauth2.Token) (cmdModel, tea.Cmd) { } func (m cmdModel) View() string { + if m.frozen { + return "" + } // show tasks in key order, skipping pending tasks to keep the ui uncluttered allDone := true tasks := make([]string, 0, len(m.tasks)) diff --git a/cmd/terraform_apply.go b/cmd/terraform_apply.go index 68222f71..8f591bd7 100644 --- a/cmd/terraform_apply.go +++ b/cmd/terraform_apply.go @@ -61,7 +61,8 @@ type tfApplyModel struct { endingChangeSnapshot snapshotModel progress []string - width int + execCommandFunc ExecCommandFunc + width int } type startStartingSnapshotMsg struct{} @@ -73,7 +74,7 @@ type changeIdentifiedMsg struct { type runTfApplyMsg struct{} type tfApplyFinishedMsg struct{} -func NewTfApplyModel(args []string) tea.Model { +func NewTfApplyModel(args []string, execCommandFunc ExecCommandFunc) tea.Model { hasPlanSet := false autoapprove := false planFile := "overmind.plan" @@ -133,7 +134,7 @@ func NewTfApplyModel(args []string) tea.Model { planFile: planFile, needPlan: !hasPlanSet, - runPlanTask: NewRunPlanModel(planArgs, planFile), + runPlanTask: NewRunPlanModel(planArgs, planFile, execCommandFunc), runPlanFinished: hasPlanSet, submitPlanTask: NewSubmitPlanModel(planFile), @@ -145,6 +146,8 @@ func NewTfApplyModel(args []string) tea.Model { endingChange: make(chan tea.Msg, 10), // provide a small buffer for sending updates, so we don't block the processing endingChangeSnapshot: NewSnapShotModel("Ending Change", "indexing resources"), progress: []string{}, + + execCommandFunc: execCommandFunc, } } @@ -243,23 +246,26 @@ func (m tfApplyModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { c.Env = append(c.Env, fmt.Sprintf("AWS_PROFILE=%v", aws_profile)) } - _, span := tracing.Tracer().Start(m.ctx, "terraform apply", trace.WithAttributes( // nolint:spancheck // will be ended in the tea.ExecProcess cleanup func + _, span := tracing.Tracer().Start(m.ctx, "terraform apply", trace.WithAttributes( // nolint:spancheck // will be ended in the tea.Exec cleanup func attribute.String("command", strings.Join(m.args, " ")), )) - return m, tea.ExecProcess( // nolint:spancheck // will be ended in the tea.ExecProcess cleanup func - c, - func(err error) tea.Msg { - defer span.End() - - if err != nil { - return fatalError{err: fmt.Errorf("failed to run terraform apply: %w", err)} - } + return m, tea.Sequence( + func() tea.Msg { return freezeViewMsg{} }, + tea.Exec( // nolint:spancheck // will be ended in the tea.Exec cleanup func + m.execCommandFunc(c), + func(err error) tea.Msg { + defer span.End() + + if err != nil { + return fatalError{err: fmt.Errorf("failed to run terraform apply: %w", err)} + } - return tfApplyFinishedMsg{} - }) + return tfApplyFinishedMsg{} + })) case tfApplyFinishedMsg: m.isEnding = true cmds = append(cmds, + func() tea.Msg { return unfreezeViewMsg{} }, m.endingChangeSnapshot.Init(), m.startEndChangeCmd(), m.waitForEndingActivity, diff --git a/cmd/terraform_plan.go b/cmd/terraform_plan.go index 6d8e162b..6d297f14 100644 --- a/cmd/terraform_plan.go +++ b/cmd/terraform_plan.go @@ -62,7 +62,7 @@ type mappedItemDiffsMsg struct { unsupported map[string][]*sdp.MappedItemDiff } -func NewTfPlanModel(args []string) tea.Model { +func NewTfPlanModel(args []string, execCommandFunc ExecCommandFunc) tea.Model { hasPlanOutSet := false planFile := "overmind.plan" for i, a := range args { @@ -97,7 +97,7 @@ func NewTfPlanModel(args []string) tea.Model { return tfPlanModel{ args: args, - runPlanTask: NewRunPlanModel(args, planFile), + runPlanTask: NewRunPlanModel(args, planFile, execCommandFunc), submitPlanTask: NewSubmitPlanModel(planFile), planFile: planFile, } From 6dbeab5f7f1ab72eae4761ef1f4616276833f81e Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 23 May 2024 15:08:40 +0200 Subject: [PATCH 07/12] Implement hideStartupStatusMsg to hid the status messages after the first command ran This allows targeted removal of the messages that persist in terminal history before the initial terraform plan or apply runs --- cmd/tea_terraform.go | 50 ++++++++++++++++++++++-------------------- cmd/terraform_apply.go | 3 +++ cmd/terraform_plan.go | 3 +++ 3 files changed, 32 insertions(+), 24 deletions(-) diff --git a/cmd/tea_terraform.go b/cmd/tea_terraform.go index 6ab38858..82ae9401 100644 --- a/cmd/tea_terraform.go +++ b/cmd/tea_terraform.go @@ -37,6 +37,8 @@ type cmdModel struct { frozen bool frozenView string // this gets set if the view is frozen, and will be used to render the last view using the cliExecCommand + hideStartupStatus bool + // business logic. This model will implement the actual CLI functionality requested. cmd tea.Model } @@ -44,6 +46,8 @@ type cmdModel struct { type freezeViewMsg struct{} type unfreezeViewMsg struct{} +type hideStartupStatusMsg struct{} + type delayQuitMsg struct{} // fatalError is a wrapper for errors that should abort the running tea.Program. @@ -108,6 +112,8 @@ func (m *cmdModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case unfreezeViewMsg: m.frozen = false m.frozenView = "" + case hideStartupStatusMsg: + m.hideStartupStatus = true case fatalError: log.WithError(msg.err).WithField("msg.id", msg.id).Debug("cmdModel: fatalError received") @@ -202,35 +208,31 @@ func (m cmdModel) View() string { if m.frozen { return "" } - // show tasks in key order, skipping pending tasks to keep the ui uncluttered - allDone := true - tasks := make([]string, 0, len(m.tasks)) - keys := make([]string, 0, len(m.tasks)) - for k := range m.tasks { - keys = append(keys, k) - } - sort.Strings(keys) - for _, k := range keys { - t, ok := m.tasks[k].(WithTaskModel) - if ok { - if t.TaskModel().status != taskStatusDone { - allDone = false - } - if t.TaskModel().status == taskStatusPending { - continue + bits := []string{} + + if !m.hideStartupStatus { + // show tasks in key order, skipping pending bits to keep the ui uncluttered + keys := make([]string, 0, len(m.tasks)) + for k := range m.tasks { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + t, ok := m.tasks[k].(WithTaskModel) + if ok { + if t.TaskModel().status == taskStatusPending { + continue + } } + bits = append(bits, m.tasks[k].View()) } - tasks = append(tasks, m.tasks[k].View()) } - if allDone { - // no need to show setup tasks after they're all done - tasks = []string{} - } - tasks = append(tasks, m.cmd.View()) + + bits = append(bits, m.cmd.View()) if m.fatalError != "" { - tasks = append(tasks, markdownToString(fmt.Sprintf("> Fatal Error: %v\n", m.fatalError))) + bits = append(bits, markdownToString(fmt.Sprintf("> Fatal Error: %v\n", m.fatalError))) } - return strings.Join(tasks, "\n") + return strings.Join(bits, "\n") } var applyOnlyArgs = []string{ diff --git a/cmd/terraform_apply.go b/cmd/terraform_apply.go index 8f591bd7..951d3a59 100644 --- a/cmd/terraform_apply.go +++ b/cmd/terraform_apply.go @@ -198,6 +198,8 @@ func (m tfApplyModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } }) } + case submitPlanNowMsg: + cmds = append(cmds, func() tea.Msg { return hideStartupStatusMsg{} }) case submitPlanFinishedMsg: cmds = append(cmds, func() tea.Msg { return startStartingSnapshotMsg{} }) @@ -266,6 +268,7 @@ func (m tfApplyModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.isEnding = true cmds = append(cmds, func() tea.Msg { return unfreezeViewMsg{} }, + func() tea.Msg { return hideStartupStatusMsg{} }, m.endingChangeSnapshot.Init(), m.startEndChangeCmd(), m.waitForEndingActivity, diff --git a/cmd/terraform_plan.go b/cmd/terraform_plan.go index 6d297f14..e310c792 100644 --- a/cmd/terraform_plan.go +++ b/cmd/terraform_plan.go @@ -132,6 +132,9 @@ func (m tfPlanModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { cmds = append(cmds, func() tea.Msg { return submitPlanNowMsg{} }) } + case submitPlanNowMsg: + cmds = append(cmds, func() tea.Msg { return hideStartupStatusMsg{} }) + case submitPlanFinishedMsg: cmds = append(cmds, func() tea.Msg { return delayQuitMsg{} }) } From 1d3c2e8209b21e1d0706ec50749d79edc057e0b6 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 23 May 2024 15:24:54 +0200 Subject: [PATCH 08/12] Remove unnecessary whitespace from main view --- cmd/tea_terraform.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/cmd/tea_terraform.go b/cmd/tea_terraform.go index 82ae9401..bd937e38 100644 --- a/cmd/tea_terraform.go +++ b/cmd/tea_terraform.go @@ -3,6 +3,7 @@ package cmd import ( "context" "fmt" + "slices" "sort" "strings" "time" @@ -230,8 +231,14 @@ func (m cmdModel) View() string { bits = append(bits, m.cmd.View()) if m.fatalError != "" { - bits = append(bits, markdownToString(fmt.Sprintf("> Fatal Error: %v\n", m.fatalError))) + md := markdownToString(fmt.Sprintf("> Fatal Error: %v\n", m.fatalError)) + md, _ = strings.CutPrefix(md, "\n") + md, _ = strings.CutSuffix(md, "\n") + bits = append(bits, fmt.Sprintf("%v", md)) } + bits = slices.DeleteFunc(bits, func(s string) bool { + return s == "" || s == "\n" + }) return strings.Join(bits, "\n") } From 28e39bf9ed3f1712cd6f74672050992cf7b3557f Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 23 May 2024 15:28:51 +0200 Subject: [PATCH 09/12] golangci-lint run ./... --- cmd/terraform_apply.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/terraform_apply.go b/cmd/terraform_apply.go index 951d3a59..6f219b35 100644 --- a/cmd/terraform_apply.go +++ b/cmd/terraform_apply.go @@ -251,9 +251,9 @@ func (m tfApplyModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { _, span := tracing.Tracer().Start(m.ctx, "terraform apply", trace.WithAttributes( // nolint:spancheck // will be ended in the tea.Exec cleanup func attribute.String("command", strings.Join(m.args, " ")), )) - return m, tea.Sequence( + return m, tea.Sequence( // nolint:spancheck // will be ended in the tea.Exec cleanup func func() tea.Msg { return freezeViewMsg{} }, - tea.Exec( // nolint:spancheck // will be ended in the tea.Exec cleanup func + tea.Exec( m.execCommandFunc(c), func(err error) tea.Msg { defer span.End() From 8faabeffb75573b137fac2db6ef1670a4d8a2ede Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 23 May 2024 15:37:22 +0200 Subject: [PATCH 10/12] rely on cmdModel's new defaults --- cmd/tea_ensuretoken.go | 6 ------ cmd/tea_revlink.go | 4 ---- cmd/tea_taskmodel.go | 6 ------ 3 files changed, 16 deletions(-) diff --git a/cmd/tea_ensuretoken.go b/cmd/tea_ensuretoken.go index 31d736c2..a5d8c8f3 100644 --- a/cmd/tea_ensuretoken.go +++ b/cmd/tea_ensuretoken.go @@ -147,12 +147,6 @@ Then enter the code: m.errors = append(m.errors, fmt.Sprintf("Note: %v", msg.err)) } return m, nil - case fatalError: - if msg.id == m.spinner.ID() { - m.status = taskStatusError - m.title = markdownToString(fmt.Sprintf("Ensuring Token Error: %v", msg.err)) - } - return m, nil default: var taskCmd tea.Cmd m.taskModel, taskCmd = m.taskModel.Update(msg) diff --git a/cmd/tea_revlink.go b/cmd/tea_revlink.go index db1ac30b..67f538d5 100644 --- a/cmd/tea_revlink.go +++ b/cmd/tea_revlink.go @@ -66,10 +66,6 @@ func (m revlinkWarmupModel) Update(msg tea.Msg) (revlinkWarmupModel, tea.Cmd) { cmds = append(cmds, m.waitForStatusActivity) case revlinkWarmupFinishedMsg: m.taskModel.status = taskStatusDone - case fatalError: - if msg.id == m.spinner.ID() { - m.taskModel.status = taskStatusError - } default: var taskCmd tea.Cmd m.taskModel, taskCmd = m.taskModel.Update(msg) diff --git a/cmd/tea_taskmodel.go b/cmd/tea_taskmodel.go index 06b413c5..27f231c5 100644 --- a/cmd/tea_taskmodel.go +++ b/cmd/tea_taskmodel.go @@ -92,12 +92,6 @@ func (m taskModel) Update(msg tea.Msg) (taskModel, tea.Cmd) { if m.spinner.ID() == msg.id { m.status = msg.status } - case fatalError: - if m.spinner.ID() == msg.id { - m.status = taskStatusError - m.title = fmt.Sprintf("%v: %v", m.title, msg.err) - return m, nil - } default: if m.status == taskStatusRunning { var cmd tea.Cmd From 5271eb97c13a1603f5995fc05165f30a269f3fb1 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 23 May 2024 15:46:32 +0200 Subject: [PATCH 11/12] avoid console flickering to allow click to be registered --- cmd/tea_ensuretoken.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/cmd/tea_ensuretoken.go b/cmd/tea_ensuretoken.go index a5d8c8f3..4a820638 100644 --- a/cmd/tea_ensuretoken.go +++ b/cmd/tea_ensuretoken.go @@ -96,7 +96,7 @@ func (m ensureTokenModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case displayAuthorizationInstructionsMsg: m.config = msg.config m.deviceCode = msg.deviceCode - + m.status = taskStatusDone // avoid console flickering to allow click to be registered m.title = "Manual device authorization." beginAuthMessage := `# Authenticate with a browser @@ -133,14 +133,17 @@ Then enter the code: case tokenLoadedMsg: m.status = taskStatusDone m.title = "Using stored token" + m.deviceMessage = "" return m, m.tokenAvailable(msg.token) case tokenReceivedMsg: m.status = taskStatusDone m.title = "Authentication successful, using API key" + m.deviceMessage = "" return m, m.tokenAvailable(msg.token) case tokenStoredMsg: m.status = taskStatusDone m.title = fmt.Sprintf("Authentication successful, token stored locally (%v)", msg.file) + m.deviceMessage = "" return m, m.tokenAvailable(msg.token) case otherError: if msg.id == m.spinner.ID() { @@ -159,7 +162,7 @@ func (m ensureTokenModel) View() string { if len(m.errors) > 0 { view += fmt.Sprintf("\n%v\n", strings.Join(m.errors, "\n")) } - if m.deviceMessage != "" && !(m.status == taskStatusDone || m.status == taskStatusError) { + if m.deviceMessage != "" { view += fmt.Sprintf("\n%v\n", m.deviceMessage) } return view From 86f937119381b4848cbfacd1272f8bc9239b42dc Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 23 May 2024 15:49:09 +0200 Subject: [PATCH 12/12] Correctly detect timeouts when waiting on a device token Based on this comment from a golang engineer, I do not expect this to be fixed quickly: > We should only use %w when the wrapper error really should be treated as an instance of the underlying error. That does happen but is not the normal case. --- cmd/tea_ensuretoken.go | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/cmd/tea_ensuretoken.go b/cmd/tea_ensuretoken.go index 4a820638..43cd4da9 100644 --- a/cmd/tea_ensuretoken.go +++ b/cmd/tea_ensuretoken.go @@ -269,32 +269,41 @@ func (m ensureTokenModel) awaitTokenCmd() tea.Msg { defer cancel() } - // while the RFC requires the oauth2 library to use 5 as the default, - // Auth0 should be able to handle more. Hence we re-implement the + // while the RFC requires the oauth2 library to use 5 as the default, Auth0 + // should be able to handle more. Hence we re-implement the m.deviceCode.Interval = 1 var token *oauth2.Token var err error for { + log.Trace("attempting to get token from auth0") // reset the deviceCode's expiry to at most 1.5 seconds m.deviceCode.Expiry = time.Now().Add(1500 * time.Millisecond) token, err = m.config.DeviceAccessToken(ctx, m.deviceCode) if err == nil { // we got a token, continue below. kthxbye + log.Trace("we got a token from auth0") break } - if errors.Is(err, context.DeadlineExceeded) { + // See https://github.com/golang/oauth2/issues/635, + // https://github.com/golang/oauth2/pull/636, + // https://go-review.googlesource.com/c/oauth2/+/476316 + if errors.Is(err, context.DeadlineExceeded) || strings.HasSuffix(err.Error(), "context deadline exceeded") { // the context has expired, we need to retry + log.WithError(err).Trace("context.DeadlineExceeded - waiting for a second") + time.Sleep(time.Second) continue } // re-implement DeviceAccessToken's logic, but faster - e, ok := err.(*oauth2.RetrieveError) // nolint:errorlint // we depend on DeviceAccessToken() returning an non-wrapped error - if !ok { + e, isRetrieveError := err.(*oauth2.RetrieveError) // nolint:errorlint // we depend on DeviceAccessToken() returning an non-wrapped error + if !isRetrieveError { + log.WithError(err).Trace("error authorizing token") return fatalError{id: m.spinner.ID(), err: fmt.Errorf("error authorizing token: %w", err)} } + switch e.ErrorCode { case "slow_down": // // https://datatracker.ietf.org/doc/html/rfc8628#section-3.5 @@ -307,7 +316,6 @@ func (m ensureTokenModel) awaitTokenCmd() tea.Msg { default: return fatalError{id: m.spinner.ID(), err: fmt.Errorf("error authorizing token (%v): %w", e.ErrorCode, err)} } - } span := trace.SpanFromContext(m.ctx)