feat: ability to list microvms based on name #369
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
== Background
I considered two routes:
1. Create a new Find() method
2. Add the name field on the List() method
The second option sounds better because it's already a List method, so
makes sense to list mirovms that way.
The extra name field is optional, if it's not provided it works the same
way as before, but if it's provided it will filter based on namespace
and name.
== Changes
=== proto and gRPC server
Changing the definition to use name in List queries.
=== tests
* Update all unit tests.
* Update e2e tests.
* Add extra case to e2e test: List request with name field and expect
only one MicroVM in the response.
=== GetAll function chain
In the background I change the whole GetAll function chain to use a
filter `map[string]string`, that way we can use them to filter based on
different filters.
I considered doing a much cleaner solution something like this:
// WithNamespace is a namespace filter for queries.
func WithNamespace(namespace string) models.ListMicroVMQuery {
return func() (string, string) {
return NamespaceLabel(), namespace
}
}
// WithName is a name filter for queries.
func WithName(name string) models.ListMicroVMQuery {
return func() (string, string) {
return NameLabel(), namespace
}
}
But at the end, I couldn't find a good place for it becasue that should
live under containerd as those labels are containerd specific formats,
but then I couldn't think of an easy way to astract it through ports and
creating an outer layer for filters and put it in ports seems
unnecessary and overkill, so I stuck with the simple key-value map.
Fixes #368
yitsushi
added
kind/feature
|
Codecov Report
@@ Coverage Diff @@
## main #369 +/- ##
=======================================
Coverage 57.37% 57.38%
=======================================
Files 51 51
Lines 2550 2567 +17
=======================================
+ Hits 1463 1473 +10
- Misses 964 970 +6
- Partials 123 124 +1
Continue to review full report at Codecov.
|
|
❯ cat hack/scripts/payload/ListMicroVMs.json \
&& echo "----" \
&& ./hack/scripts/send.sh -m ListMicroVMs \
| dasel -p json --multiple '.microvm.[*].spec.uid'
{
"namespace": "ns1",
"name": "mvm2"
}
----
"01FTBC5ZEKE79FGYNRHPC8MME5"
"01FTBFG9KAC46TVDYGH1JMH665"
❯ cat hack/scripts/payload/ListMicroVMs.json \
&& echo "----" \
&& ./hack/scripts/send.sh -m ListMicroVMs \
| dasel -p json --multiple '.microvm.[*].spec.uid'
{
"namespace": "ns1"
}
----
"01FTBFG9KAC46TVDYGH1JMH665"
"01FTBC51BK5V82XEDBASXAF61Z"
"01FTBC5ZEKE79FGYNRHPC8MME5"
Fixes #370
|
Callisto13
reviewed
Jan 26, 2022
| @@ -409,11 +409,18 @@ func TestApp_GetAllMicroVM(t *testing.T) { | |||
| expect func(rm *mock.MockMicroVMRepositoryMockRecorder, em *mock.MockEventServiceMockRecorder, im *mock.MockIDServiceMockRecorder, pm *mock.MockMicroVMServiceMockRecorder) | |||
| }{ | |||
| { | |||
| name: "empty namespace should return an error", | |||
| name: "empty namespace should not return an error", | |||
There was a problem hiding this comment.
do we want equivalent test for name?
There was a problem hiding this comment.
A lot of them are testing that. And actually there is not test that receives the name.
Callisto13
approved these changes
Jan 26, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Background
I considered two routes:
The second option sounds better because it's already a List method, so
makes sense to list mirovms that way.
The extra name field is optional, if it's not provided it works the same
way as before, but if it's provided it will filter based on namespace
and name.
Changes
proto and gRPC server
Changing the definition to use name in List queries.
tests
only one MicroVM in the response.
GetAll function chain
In the background I change the whole GetAll function chain to use a
filter
map[string]string, that way we can use them to filter based ondifferent filters.
I considered doing a much cleaner solution something like this:
But at the end, I couldn't find a good place for it becasue that should
live under containerd as those labels are containerd specific formats,
but then I couldn't think of an easy way to astract it through ports and
creating an outer layer for filters and put it in ports seems
unnecessary and overkill, so I stuck with the simple key-value map.
Fix list results
List endpoints returned only one of the MicroVMs when we have multiple MicroVMs with the same ns/name.
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 #368
Fixes #370
Special notes for your reviewer: