Add Spinner implementation for longer-running operations. by grayside · Pull Request #115 · phase2/rig

grayside · 2017-11-07T21:18:50Z

There are a number of operations that rig command execute which can take awhile, which is exacerbated by our use of the --verbose flag to keep some of the inner workings terse. As an example, a rig start can go almost a minute without a single terminal output if --verbose is not used and several retries are needed to get the docker machine up.

This PR adds a new util/spinner.go mechanism which wraps a new library that provides spinner support. If rig is run in verbose mode, it will pass messages sent to the spinner to the logger, if not, it will output the messages in conjunction with a spinner lifecycle to help communicate when rig is managing a longer-running process.

This PR attempts to cover all the bases, with exceptions in rig project sync and machine.go::WaitForDev, which seemed just as well as follow-ups.

febbraro · 2017-11-09T18:16:55Z

+// When verbose, we perform basic logging instead of running the spinner.
+func (s *RigSpinner) Start(message string) {
+	if Logger().IsVerbose {
+		Logger().Info.Println(message)


I'm not liking the output in verbose mode with the "Starting..." and the end state. What do you think about not not showing the start message, just the complete messages on Verbose. Those start messages were not really included in output before during verbose so now it is even MORE verbose, almost....annoyingly?

The problem I found without the start message is that the spinner message would sometimes lack context. Let me audit/update the messages to see if there's enough context in verbose mode withotu the start

I think when the spinner is in place you should use the start message, but when in verbose mode, just do nothing.

* origin/develop: Display executed commands via Verbose Logging (#111) Better cross platform builds (#118) Fixing Outrigger Dashboard on Linux (#117) Fixed bug in libnss-resolver DNS resolution (#116) Added rpm to build, well, rpms Transitioned to different go base to try to head off problems with dynamically linked go binaries tweaking some build flags tweaking CGO messing with ldflags tweaked goreleaser config

grayside · 2017-11-15T19:51:55Z

+* Make sure your branch conforms with go fmt standards.
+* Manually test your changes.
+
+## User Interactions


New CONTRIBUTING guide, which includes thoughts on logging that illustrates some of the API that's coming together.

grayside · 2017-11-15T19:52:31Z

+* **Use the correct method to log operational results: (Pick one)**
+  * `cmd.out.Success("Sauce is Ready.")`
+  * `cmd.out.Warn("Sauce is burnt on the bottom.")`
+  * `cmd.out.Oops("Discard this sauce and try again.")`


Oops is temporary, there are a number of "refactoring in progress" things going on before all the specific API details are right.

febbraro · 2017-11-15T20:02:16Z

 	}
 	color.Unset()
+	cmd.out.Success("Networking cleanup completed")
+	cmd.out.NoSpin()


No need to NoSpin() here, it's handled in BaseCommand.Success() no?

febbraro · 2017-11-15T20:13:19Z

 		Warning:   log.New(os.Stdout, color.YellowString("[WARN] "), 0),
 		Error:     log.New(os.Stderr, color.RedString("[ERROR] "), 0),
 		Verbose:   log.New(verboseWriter, "[VERBOSE] ", 0),
+		Message:   log.New(os.Stdout, " - ", 0),


This doesn't appear to be used

Sorry, i see where it is used. What is the idea behind Message?

It will be pulled if continuing refactor doesn't find a use for it. I wanted to stake some concepts aside from log severity for the channels.

febbraro · 2017-11-15T20:15:06Z

 // Log verbosely logs the command.
 func (x Executor) Log(tag string) {
-	color.Set(color.FgYellow)
+	color.Set(color.FgMagenta)


Was this was already merged?

I made that change here, then created a separate branch, and didn't pull it back out. This change made it into v2.

febbraro · 2017-11-15T20:16:14Z

This is much more along the lines of what I was thinking.

febbraro · 2017-11-15T21:23:50Z

+* **Command has executed and is successful. We do not want a notification:**
+  ```
+  cmd.out.Success("Enjoy your dinner.")
+  return cmd.Success("")


cmd.out.Success() and cmd.Success() is confusing. Can they be consolidated?, or can we rename one?

We may also want a SilentSuccess() so we dont need to send in ""?

yeah, or SuccessWithNotification()

I agree the two Success() statements is confusing. I originally had it as Complete(), but complete only made sense in the context of the spinner. Unfortunately this "success" is often the equivalent of an info log. I will look at the API overall and see what can be rejiggered. Now that I know I'm on the right track I can make deeper cuts.

grayside · 2017-11-17T01:01:19Z

On the idea that this API has mostly solidified, but is going to have git conflicts if you blow on it, might we merge this and complete polishing the experience as a follow-up?

tekante · 2017-11-22T15:13:32Z

+}
+
+// Verbose allows Verbose logging of more advanced activities/information.
+// In practice, if the spinner can be in use verbose is a no-op.


Having a little trouble wrapping my brain around this comment. Is this saying that if the spinner is running verbose output won't be seen? Is that an indicator that using the --verbose flag indicates that the spinner won't run? That is my intuition but I haven't been able to perceive in the code what makes that the case.

If you run rig --verbose, you get "classic" logging. The text from Spin() is not output in verbose mode.

tekante · 2017-11-22T15:16:20Z

+  * `cmd.out.Info("Sauce is Ready.")`
+  * `cmd.out.Warning("Sauce is burnt on the bottom.")`
+  * `cmd.out.Error("Discard this sauce and try again.")`
+* **Going to send some contextual notes to the user**:


Is this example one where you always want to send a note to the user (or is the info/verbose a distinction on whether it's always versus only if --verbose is set)? If it's an indicator that the spinner needs to be disabled in those cases should we put re-enabling it after sending the message?

I don't see any indications of using NoSpin in the code so far other than in a Failure message so that makes me think perhaps I'm not interpreting how/when it needs to be used in order to use it correctly.

It could be that NoSpin needs to be added somewhere. If you do some regular logs when the Spinner is active, it suspends the spinner on line 1, outputs the log on line 2, and resumes the spinner on line 3. If you add NoSpin before logging, it does not resume on line 3.

Add Spinner implementation for longer-running operations.

5f2c6e7

grayside added the enhancement label Nov 7, 2017

grayside requested review from febbraro and tekante November 7, 2017 21:18

grayside added the in progress label Nov 7, 2017

Adam Ross added 2 commits November 7, 2017 13:22

Correct syntax of spinner library lock entry.

84aedd3

Fixed up go formatting.

7a1f991

febbraro reviewed Nov 9, 2017

View reviewed changes

Adam Ross added 2 commits November 10, 2017 17:15

Refactoring towards a new logging API.

75ca4c5

grayside commented Nov 15, 2017

View reviewed changes

febbraro reviewed Nov 15, 2017

View reviewed changes

Adam Ross and others added 6 commits November 16, 2017 15:41

Refactor codebase to use existing log API via RigLogger.Channel.*

fcfb442

Move fmt.Sprintf into the logging API. Rename cmd.Error to cmd.Failure

a717a33

Merge branch 'develop' into feature/spinners

125a894

Fixes for go fmt and lint.

7cb8bf9

Update CONTRIBUTING.md for API changes.

f10cecb

Another round of go fmt changes.

140b317

febbraro approved these changes Nov 17, 2017

View reviewed changes

febbraro merged commit e11e398 into develop Nov 17, 2017

febbraro deleted the feature/spinners branch November 17, 2017 03:50

grayside added this to the v2.1 milestone Nov 17, 2017

tekante reviewed Nov 22, 2017

View reviewed changes

febbraro removed the in progress label Feb 13, 2018

Conversation

grayside commented Nov 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

febbraro commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grayside commented Nov 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants