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

feat: added microvm spec & repository #34

Merged
merged 2 commits into from
Jul 28, 2021
Merged

feat: added microvm spec & repository #34

merged 2 commits into from
Jul 28, 2021

Conversation

richardcase
Copy link
Member

@richardcase richardcase commented Jul 28, 2021

What this PR does / why we need it:

Added API definition for a MicroVM and a MicroVMList. Created a repository that do get/save/delete instances of the api. A
implementation that uses the containerd content store has been added.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #24
Fixes #25

Special notes for your reviewer:

Checklist:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Release note:

MicroVM API definition added along with a containerd based repo to store instances of the microvm definitions.

Added API definition for a MicroVM and a MicroVMList. Created
a repository that do get/save/delete instances of the api. A
implementation that uses the containerd content store has been
added.

Signed-off-by: Richard Case <[email protected]>
@richardcase richardcase added the kind/feature New feature or request label Jul 28, 2021
Copy link

@bigkevmcd bigkevmcd left a comment

Choose a reason for hiding this comment

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

Looks good, nothing major.

Makefile Outdated Show resolved Hide resolved
pkg/repository/microvm.go Outdated Show resolved Hide resolved
"github.com/weaveworks/reignite/pkg/defaults"
)

var (

Choose a reason for hiding this comment

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

These are exported, should probably have a doc comment?

pkg/repository/microvm.go Outdated Show resolved Hide resolved
)

// MicroVM is the repoitory definition for a microvm repository.
type MicroVM interface {

Choose a reason for hiding this comment

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

This might be better named Store or Repository or something similar to avoid MicroVM being both a place to store things, and a thing to be stored there?

pkg/repository/microvm.go Outdated Show resolved Hide resolved
ctx := context.Background()

store, contentDir := getLocalContentStore(t)
defer os.RemoveAll(contentDir)

Choose a reason for hiding this comment

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

You could use t.Cleanup() here?

(bonus points for adding it in the getLocalContentStore function, because it would be triggered when the test was being cleaned up).

pkg/repository/microvm.go Outdated Show resolved Hide resolved
pkg/repository/microvm_test.go Outdated Show resolved Hide resolved
api/reignite/v1alpha1/microvm.go Show resolved Hide resolved
Various changes as a result of code review, inclduing:

* Changing the name of the repository interface
* Cleaning up temporary dis during testing.

Signed-off-by: Richard Case <[email protected]>
@richardcase richardcase merged commit 4789cf1 into main Jul 28, 2021
@richardcase richardcase deleted the mvm_repo branch July 28, 2021 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request size/l
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create vm spec storage implementation Create vm spec storage interface
2 participants