reorganize virtwrap/cache NamespaceKey functions #354
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
|
Can one of the admins verify this patch? |
|
Maybe point for discussion: I'm thinking of moving the libvirt connection handling under cache while leaving the manager (KillVM, SyncVM, iface, helpers) stay in virtwrap. Opinions? |
mpolednik
force-pushed
the
cachenamespacekeyfunc
branch
from
59c78c7
to
4c6a0d0
Compare
rmohr
requested changes
Aug 3, 2017
pkg/virt-handler/rest/console.go
Outdated
| @@ -52,7 +52,7 @@ func (t *Console) Console(request *restful.Request, response *restful.Response) | |||
| namespace := request.PathParameter("namespace") | |||
| vm := v1.NewVMReferenceFromNameWithNS(namespace, vmName) | |||
| log := logging.DefaultLogger().Object(vm) | |||
| domain, err := t.connection.LookupDomainByName(virtwrap.VMNamespaceKeyFunc(vm)) | |||
| domain, err := t.connection.LookupDomainByName(virtwrap.VmNamespaceKeyFunc(vm)) | |||
There was a problem hiding this comment.
Go uses VMNamespaceKeyFunc, see https://github.com/golang/go/wiki/CodeReviewComments#initialisms
There was a problem hiding this comment.
Cool, couldn't find this source. Will drop the renaming for sure!
Maybe moving it to a cli package alongside cache? |
The errors can live in their own package to avoid circular dependencies inside (and even outside of) virtwrap. Signed-off-by: Martin Polednik <mpolednik@redhat.com>
mpolednik
force-pushed
the
cachenamespacekeyfunc
branch
from
4c6a0d0
to
3d982d3
Compare
|
Took a bit longer approach to the move. The PR now contains commit that
|
mpolednik
force-pushed
the
cachenamespacekeyfunc
branch
from
3d982d3
to
e054790
Compare
|
ok to test |
rmohr
approved these changes
Aug 4, 2017
rmohr
requested changes
Aug 4, 2017
The file manager.go was contained two loosely related themes of functionality: the DomainManager and some libvirt connectivity. The libvirt part can be isolated under cli, allowing us to treat is just as another client within KubeVirt. Signed-off-by: Martin Polednik <mpolednik@redhat.com>
k8s/meta/v1 was imported twice under different names, there is no reason to have such pollution in the namespace at the moment. Signed-off-by: Martin Polednik <mpolednik@redhat.com>
Mirroring the k8s client, VMNamespaceKeyFunc and SplitVMNamespaceKey should belong together under one package. Cache was chosen as it is that way in k8s/client-go. Signed-off-by: Martin Polednik <mpolednik@redhat.com>
mpolednik
force-pushed
the
cachenamespacekeyfunc
branch
from
e054790
to
85b8a4f
Compare
kubevirt-bot
pushed a commit
to kubevirt-bot/kubevirt
that referenced
this pull request
Nov 6, 2020
Add regular PVC creation e2e test.
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.
The functions had two issues: they had been placed in different packages and didn't really follow golang's naming convention. This PR solves both, moving the functions to virtwrap package.
The final destination should be the cache package, but that requires more refactoring to avoid cyclic imports.