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

Update Commands to use libhostmgr #51

Merged
merged 16 commits into from
Jan 10, 2023
Merged

Update Commands to use libhostmgr #51

merged 16 commits into from
Jan 10, 2023

Conversation

jkmassel
Copy link
Contributor

@jkmassel jkmassel commented Jan 5, 2023

Builds on #46 by migrating the hostmgr CLI commands to use libhostmgr under the hood.

This uses far less code to do the job, and finishes threading async support all the way up to the UI layer.

To Test:
Running each command should be a good way to test – you can try:

  • swift run hostmgr vm list [should emit a list of VMs, local and remote]
  • swift run hostmgr vm exists xcode-99 [should exit with an error saying the VM doesn't exist]
  • swift run hostmgr vm clean [should run with empty output]
  • swift run hostmgr vm fetch xcode-14.2 [should download the VM and unpack it]
  • swift run hostmgr vm start xcode-14.2 [should fire up the VM]
  • swift run hostmgr vm stop --all --immediately [should immediately pull the plug on the VM]

@jkmassel jkmassel added the enhancement New feature or request label Jan 5, 2023
@jkmassel jkmassel requested a review from a team January 5, 2023 01:56
@jkmassel jkmassel self-assigned this Jan 5, 2023
Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

None of my comments are blocking.

Unfortunately, I wasn't able to test this properly because "Error: InvalidAccessKeyId: The AWS Access Key Id you provided does not exist in our records."

Comment on lines 7 to 10
struct Constants {
static let s3Key = "authorized_keys"
static let destination = Configuration.shared.localAuthorizedKeys
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick. I once saw this kind of information container type being an enum, which I found neat because it prevents doing _ = Constants().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neat! Done in e7eabdb

}

// In the case of invalid input like this, show the help text
guard let identifier = self.identifier else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick. This could become shorter thanks to SE-0345, see wordpress-mobile/WordPress-iOS#19549

Suggested change
guard let identifier = self.identifier else {
guard let identifier else {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah nice! I've been using it for if let, but I guess it makes sense this would also work for guard let!

Cleaned up in 3c22054

.forEach { try $0.shutdown(immediately: immediately) }
if all {
try libhostmgr.stopAllRunningVMs(immediately: self.immediately)
Console.exit()
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to add a return here, just in case something changes in Console.exit()?

If so, what do you think of using guard all == false else ... to ensure return is called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm interesting – I wouldn't expect Console.exit() to ever do anything except stop the application. It has the return type Never, so it's just as valid as throws for breaking out of the current scope, and the type system ensures that we can't accidentally change this.

That said, using guard here would allow the type system to enforce this for us, so I've done that in b23a49e.

Sources/hostmgr/commands/vm/VMFetch.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@crazytonyli crazytonyli left a comment

Choose a reason for hiding this comment

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

I've verified all commands listed in the PR description work on my mac. 👍

@jkmassel jkmassel merged commit 314c1eb into trunk Jan 10, 2023
@jkmassel jkmassel deleted the update/vm-commands branch January 10, 2023 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants