Skip to content

Conversation

@LawnGnome
Copy link
Contributor

This PR adds a new flag — -vv — to show every command that is spawned while executing a campaign. Here it is in action:

out

If only the implementation was that simple...

Unfortunately, printing log messages via Verbose* and Write* is a bit fraught right now in internal/output: the only safe way to do it is on the active "widget" (ie Progress, or Pending, or whatever), lest your message be overwritten the next time the widget is drawn. However, actually tracking what the current widget is not terribly easy, and that's only exacerbated when it's being handled in a callback from the service type.

The naïve solution here would be to track the current widget in Output and delegate to that widget's equivalent function for Verbose* and Write* calls, but that is also problematic, since they may recursively call Output.Write and friends. (And they do.) The architectural problem here is that Output really addresses two or three separate concerns: providing an API for external users, providing an API for internal users, and handling a lot of the quirks of outputting things to various terminal types.

I had about four attempts at splitting it up today, and just couldn't get it. It's a ball of yarn, and every time I thought I had the boundaries sketched out, I found something else that needed to cross those boundaries: how the output behaves has to depend on the terminal capabilities, and the output options, and has to be lockable at the entry point into the output package so that widgets can make atomic updates. The real fix here is probably to redesign how we get characters and control codes to the screen with something like a command list that can write to a buffer and then flush to the terminal asynchronously. (This would also fix some flicker issues we have that are more noticeable on Windows.)

So, instead, here's a much hackier approach. The progress bar widgets currently draw and leave the cursor at the bottom of the drawn content, but there's no particular reason this has to be the case. If we switch them to leave the cursor at the top of the drawn content after a draw, then a verbose log line can simply overwrite that line, and then the next widget draw will put everything back the way it should be. Those tend to be common, particularly when spinners are enabled, but it does increase flickering. (Of course, that's only if -v is enabled, and it's only really bad if -vv is enabled.)

Copy link
Contributor

@mrnugget mrnugget left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for doing this!

Two thoughts I had while testing this locally:

  1. Oh, wow, impressive that a terminal UI can do this.
  2. Does it even make sense to display the normal status bars when we're in -vv mode?

The reason for the second thought is that it can feel really flickery when there are a lot of commands being executed and the UI is updating all the time.

There's also some leftover from some progress bar lines showing up:

image

I'm not saying this is bad or that we shouldn't do this, but just want to point out that we might have an easier option where -vv means: instead of using the normal progress printer, just use a really verbose logger that logs commands and some overall progress.

Since the main goal of this is to help with debugging and not to provide more information in a normal workflow I think that would be totally fine.

` + graphql.CampaignFieldsFragment

func (svc *Service) ApplyCampaign(ctx context.Context, spec CampaignSpecID) (*graphql.Campaign, error) {
ctx = svc.decorateContext(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker: would it make sense to save the context on the *Service instead of passing it into every method? I don't think we call once instance of *Service multiple times with different contexts, so caching it might make more sense vs. having the risk of forgetting to decorate the context every time.

"os/exec"
)

type OnCommand func(*exec.Cmd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick/not-a-blocker: I think CommandWrapper might be a better name here, since it's not a callback (which I always associate with "on" naming, which makes me think of events), but it does take in the command and execute it.

@LawnGnome
Copy link
Contributor Author

I think we're agreed after our sync yesterday that this is going to be replaced by some sort of debug logging instead, so closing. I might salvage some of the command hooking stuff, but that's not making me happy right now either.

@LawnGnome LawnGnome closed this Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants