-
Notifications
You must be signed in to change notification settings - Fork 126
RSDK-12022 + RSDK-12276 Provide better progress messaging for hot reloading #5418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RSDK-12022 + RSDK-12276 Provide better progress messaging for hot reloading #5418
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this looks great. thanks for putting this together! I have a couple comments (mostly minor and optional) though I would like to continue respecting NoProgress or else not advertise it as a thing we do.
| } | ||
|
|
||
| // Fail marks a step as failed with an error message. | ||
| func (pm *ProgressManager) Fail(stepID string, err error) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(minor, opt) this and FailWithMessage are largely duplicative, might be nice to factor out the shared bits. But also I think this will be touched infrequently enough that it's probably fine to just leave as is.
cli/progress_manager_test.go
Outdated
| } | ||
|
|
||
| elapsed := time.Since(step.startTime) | ||
| if elapsed < 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(q) would it be useful to instead check that elapsed is greater than 10milis (the sleep time)?
cli/progress_manager_test.go
Outdated
| // We can't easily verify the text was updated since pterm doesn't expose it, | ||
| // but we can verify no error occurred |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we should be doing some sort of check after this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, this doesn't return an error. I think I will omit the test here since we cant assert anything.
| } | ||
| err = vc.copyFilesToFqdn( | ||
| part.Part.Fqdn, globalArgs.Debug, false, false, []string{buildPath}, | ||
| dest, logging.NewLogger(reloadVersionPrefix), args.NoProgress) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was the only location where we used NoProgress for module reloading. I think ideally we should not create the progress manager if NoProgress is true, and have all PM methods return be no-ops if the pm pointer is nil. Barring that, we should probably remove NoProgress from the args type so we don't try to parse it, and make it hidden (though still existent for backwards compatibility) in the viam module reload args list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I was too focused on the goal of adding a spinner and forgot about this. Let me reincorporate NoProgress to not make there be a progress spinner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok --no-progress is now respected with the progress manager. I encapsulated it within the progress manager to avoid some gross if/else logic throughout everywhere a status is being set/updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice lgtm!
… into ml/progress-spinner-2
The changes in this PR focus on hot reloading and provide more feedback as to what the process is doing and how long it is taking.
ptermas it seems like the best golang option for multi-step progress with support to nest some steps inside of others. I did try other dependencies as well and this seems the best and most popular option. I know the CLI generator uses huh but it does not provide the same features such as multiple steps (on different lines) and step nesting. Maybe a separate task could be done to use this new dependency on the generator side.pterm.Testing
Success case
Failed build case
Prompt to create registry item case
Failed to copy to part case