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.

Sign up for GitHub

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

Jump to bottom

feat: implement query methods and add status to query api #173

Merged
merged 5 commits into from
Oct 29, 2021
Merged

feat: implement query methods and add status to query api #173

merged 5 commits into from
Oct 29, 2021

Conversation

jmickey
Copy link
Contributor

@jmickey jmickey commented Oct 27, 2021 •
edited
Loading

What this PR does / why we need it:

Adds implementation for query methods and microvm status for query gRPC API

Which issue(s) this PR fixes:
Fixes #72

Checklist:

  • squashed commits into logical changes
  • tests

@jmickey jmickey added kind/feature New feature or request area/api Indicates an issue or PR relates to the APIs labels Oct 27, 2021
@jmickey jmickey self-assigned this Oct 27, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2021 •
edited
Loading

Codecov Report

Merging #173 (fdabd8b) into main (94ab549) will increase coverage by 7.78%.
The diff coverage is 72.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #173      +/-   ##
==========================================
+ Coverage   47.00%   54.78%   +7.78%     
==========================================
  Files          45       45              
  Lines        1951     1986      +35     
==========================================
+ Hits          917     1088     +171     
+ Misses        930      788     -142     
- Partials      104      110       +6
Impacted Files Coverage Δ
core/application/errors.go 0.00% <ø> (ø)
core/models/volumes.go 0.00% <ø> (ø)
core/application/query.go 74.07% <72.00%> (+74.07%) ⬆️
infrastructure/firecracker/config.go 0.00% <0.00%> (ø)
infrastructure/containerd/event_service.go 59.64% <0.00%> (+3.50%) ⬆️
core/steps/runtime/dir_create.go 73.07% <0.00%> (+30.76%) ⬆️
core/steps/runtime/initrd_mount.go 100.00% <0.00%> (+100.00%) ⬆️
core/steps/runtime/kernel_mount.go 100.00% <0.00%> (+100.00%) ⬆️
core/steps/runtime/volume_mount.go 100.00% <0.00%> (+100.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3217c47...fdabd8b. Read the comment docs.

@jmickey jmickey requested a review from a team October 27, 2021 11:23
yitsushi
yitsushi requested changes Oct 27, 2021
View reviewed changes
core/application/query.go Outdated Show resolved Hide resolved
@yitsushi
Copy link
Contributor

And same as for list, we can add tests on this function?

richardcase
richardcase reviewed Oct 27, 2021
View reviewed changes
core/application/query.go Outdated Show resolved Hide resolved
core/application/query.go Show resolved Hide resolved
@Callisto13
Copy link
Member

the models.MicroVm is translated to a mvmv1.GetMicroVMResponse which contains *types.MicroVMSpec but not the status so that is not returned to the end user. Was this deliberate or can we update the response to also have the MicroVMStatus? would be useful 😄

@jmickey jmickey force-pushed the feat/get-microvm-api branch from 6629990 to a41b21a Compare October 28, 2021 02:54
@jmickey
feat: add MicroVM status to get and list API calls
89495b1 
- Added new methods to convert a model to MicroVMStatus
- Added JSON tags to status struct fields
- Generated new protobuffer code with Get/List request changes
@github-actions github-actions bot added size/l and removed size/xs labels Oct 28, 2021
@jmickey jmickey mentioned this pull request Oct 29, 2021
Closed
1 task
@jmickey jmickey changed the title feat: implement single microvm query API feat: implement query methods and add status to query api Oct 29, 2021
Callisto13
Callisto13 reviewed Oct 29, 2021
View reviewed changes
core/application/app_test.go Outdated Show resolved Hide resolved
core/application/app_test.go Show resolved Hide resolved
core/application/app_test.go Show resolved Hide resolved
core/application/app_test.go Outdated Show resolved Hide resolved
infrastructure/grpc/convert.go Outdated Show resolved Hide resolved
infrastructure/grpc/convert.go Outdated Show resolved Hide resolved
infrastructure/grpc/convert.go Show resolved Hide resolved
infrastructure/grpc/convert.go Show resolved Hide resolved
@jmickey jmickey force-pushed the feat/get-microvm-api branch from fdabd8b to 3d173fb Compare October 29, 2021 14:21
Callisto13
Callisto13 previously approved these changes Oct 29, 2021
View reviewed changes
Copy link
Member

@Callisto13 Callisto13 left a comment

Choose a reason for hiding this comment

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

couple small notes but lgtm 👍

core/application/app_test.go Outdated Show resolved Hide resolved
core/application/app_test.go Outdated Show resolved Hide resolved
@jmickey jmickey force-pushed the feat/get-microvm-api branch from 3d173fb to 3348eae Compare October 29, 2021 14:31
@jmickey jmickey requested a review from yitsushi October 29, 2021 14:36
@jmickey jmickey merged commit b8f6f59 into main Oct 29, 2021
@jmickey jmickey deleted the feat/get-microvm-api branch October 29, 2021 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardcase richardcase richardcase left review comments

@Callisto13 Callisto13 Callisto13 approved these changes

@yitsushi yitsushi yitsushi approved these changes

Assignees

@jmickey jmickey

Labels
area/api Indicates an issue or PR relates to the APIs kind/feature New feature or request size/l
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement query gRPC methods
5 participants
@jmickey @codecov-commenter @yitsushi @Callisto13 @richardcase