Skip to content

Update/streamline operations - #10

Open
bxffour wants to merge 2 commits into
mainfrom
update/streamline-operations
Open

Update/streamline operations#10
bxffour wants to merge 2 commits into
mainfrom
update/streamline-operations

Conversation

@bxffour

@bxffour bxffour commented Aug 12, 2023

Copy link
Copy Markdown
Contributor

Addresses: #9

This commit refactors most of the codebase, isolation several core logic into their own packages. The init command was added to ensure all dependencies are available. The create command was added to create microvms and the run command was added to run microvms.

New commands

Init

This ensures all the dependencies are available

# spitfire init

output:

2023/08/12 17:06:27 INFO init binary is ready to go
2023/08/12 17:06:27 INFO vmlinux binary is ready to go
2023/08/12 17:06:27 INFO firectl binary is ready to go

init

Create

This command creates the microvm image from a container image

  • To create just the rootfs
# spitfire create --image alpine:latest test
  • To create the init drive to go alongside the rootfs
spitfire create --init --image alpine:latest test

create

Run

This command runs the microvm

# spitfire run <image name>

run

This commit refactors most of the codebase, isolation several core logic
into their own packages. The init command was added to ensure all
dependencies are available. The create command was added to create microvms
and the run command was added to run microvms.
Comment thread cmd/create.go
image, _ := cmd.Flags().GetString("image")

if len(image) == 0 {
return fmt.Errorf("image flag must be specified")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • might be wise not to tie the create command to only creating drives from OCI images. the constants are the fstype, size and name (or path to put it). whether we want an image burned into the loop drive should be optional

@bxffour bxffour Aug 13, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting that we pull container images without making drives out of them?

Comment thread cmd/init.go
func MakeInitCmd() *cobra.Command {
command := &cobra.Command{
Use: "init",
Short: "download assets if they don't exist",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • i don't think an init command should be downloading stuff from the internet. the name does not imply that, command should imply the action they perform, so pull/download will be better for things that pull from the internet. you know where my mind is at

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree. What name would you suggest though. The idea is that it's a command that makes sure that all dependencies are met

Comment thread cmd/init.go
}

initPath := filepath.Join(assetsDir, "init")
_, err = os.Stat(initPath)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is a utils.Exists function

@thi-startup thi-startup deleted a comment from Joe-Degs Aug 13, 2023
@blackprince001

Copy link
Copy Markdown

LGTM. great work bro

@buabaj

buabaj commented Aug 21, 2023

Copy link
Copy Markdown

LGTM bro
however, it would be much better if you made smaller PRs to ensure effective reviews as large PRs are pretty clumsy and make thorough reviewing a pain.
Also, could you consider modularizing most of your code into reusable functions to prevent duplication and ensure consistency?
i also believe you can do more effective validation and error-handling for certain things like parsing in flag values, etc(make sure you don't get any expected behaviour if users don't parse flags you're expecting).
Logging and messaging also don't appear to be enough, it's fine if you want to do those later but please don't overlook it.

otherwise, everything else lgtm as I mentioned. you should as well look into Joe's comments. I think he makes a good argument about whether or not having an image burned on the drive is optional (unless I'm missing some context)

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