Skip to content
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

fix: Show in-progress spinner when creating VM #161

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hariohmprasath
Copy link

Issue: #157

Signed-off-by: Hari Ohm Prasth Rajagopal harrajag@amazon.com

Issue #, if available:
#157

Description of changes:
Show spinner when VM creation is in progress

Testing done:
Ran local tests by building the code using make command and tested it by creating a new VM finch vm init command

INFO[0000] Initializing and starting Finch virtual machine...
⣾VM creation in progress
  • I've reviewed the guidance in CONTRIBUTING.md

License Acceptance

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@davidhsingyuchen davidhsingyuchen left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I think we can merge it as it's definitely an improvement, but I'm not 100% sure if #157 can be closed by it as strictly speaking, we're not showing the "progress" (i.e., 30%, 50%, etc.). @ningziwen WDYT?

LGTM overall, just please fix the failed checks. You can ignore the e2e ones as those failures are likely caused by flakiness and are tracked in #140.

iva.logger.Errorf("Finch virtual machine failed to start, debug logs: %s", logs)
return err
}
s.Stop()

Choose a reason for hiding this comment

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

You've got this twice - consider using a defer s.Stop() instead after s.Start()

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, updated the code.

Copy link
Contributor

@davidhsingyuchen davidhsingyuchen Jan 20, 2023

Choose a reason for hiding this comment

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

Currently the logs are printed before the spinner stops, while we may want them to be printed after instead.

	if err != nil {
		iva.logger.Errorf("Finch virtual machine failed to start, debug logs: %s", logs)
		return err
	}
	iva.logger.Info("Finch virtual machine started successfully")

As a result, could you wrap the following statements in a func (initVM?) and print the logs above after calling that func? Thanks!

	instanceName := fmt.Sprintf("--name=%v", limaInstanceName)
	limaCmd := iva.creator.CreateWithoutStdio("start", instanceName, iva.baseYamlFilePath, "--tty=false")
	iva.logger.Info("Initializing and starting Finch virtual machine...")
	// Show spinner
	s := spinner.New(spinner.CharSets[11], 100*time.Millisecond)
	s.Suffix = "VM creation in progress"
	s.Start()
	defer s.Stop()
	logs, err := limaCmd.CombinedOutput()

@hariohmprasath hariohmprasath changed the title Show in-progress spinner when creating VM fix: Show in-progress spinner when creating VM Jan 20, 2023
Issue: runfinch#157

Signed-off-by: Hari Ohm Prasth Rajagopal <harrajag@amazon.com>
@hariohmprasath
Copy link
Author

WDYT

Thanks @davidhsingyuchen, I have taken care of the comments.

Regarding the progress: I looked into this code and seems like lima doesn't provide any async updates on the VM creation process. One option is to pipe the stdout from the creation progress to notify user that something is happening, but not sure whether thats the right thing to do. If you have any ideas let me know, happy to work on another PR. Thanks

@davidhsingyuchen
Copy link
Contributor

Regarding the progress: I looked into this code and seems like lima doesn't provide any async updates on the VM creation process. One option is to pipe the stdout from the creation progress to notify user that something is happening, but not sure whether thats the right thing to do. If you have any ideas let me know, happy to work on another PR. Thanks

@hariohmprasath Thanks for the investigation. I think the current form is good enough. We may not want to pipe the stdout from the creation progress as we cannot really control what's being printed out there, and it's sort of implementation details. /cc @ningziwen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants