Refactor Pool-manager tests #448
Conversation
Signed-off-by: Ram Lavi <ralavi@redhat.com>
RamLavi
force-pushed
the
refactor_pool
branch
from
8a641a4 to
e1b4ec7
Compare
The networks and network-status annotations consts are set on multus repo, and controlled by it. Using their consts to keep consistency. For example, the deprecated 'k8s.v1.cni.cncf.io/networks-status' annotation is replaced by the new one 'k8s.v1.cni.cncf.io/network-status' Signed-off-by: Ram Lavi <ralavi@redhat.com>
There is no good reason to use these functions as closure functions. Moving them to be regular functions, thus reducing code complexity. Signed-off-by: Ram Lavi <ralavi@redhat.com>
Currently not all unit tests check macpool size after doing changes in the macpool map (allocate, update or delete MACs entries) Moving the check inside checkMacPoolMapEntries so that all tests check this, removing the now redundant checks. Also refactoring use of "errors.New(fmt.Sprintf(...))" into "fmt.Errorf(...)" Signed-off-by: Ram Lavi <ralavi@redhat.com>
RamLavi
force-pushed
the
refactor_pool
branch
from
e1b4ec7 to
2574f61
Compare
pkg/pool-manager/pool_test.go
Outdated
| managedPodWithMacAllocated := v1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "podpod", Namespace: managedNamespaceName, Annotations: afterAllocationAnnotation(managedNamespaceName, managedNamespaceMAC)}} | ||
| unmanagedPodWithMacAllocated := v1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "unmanagedPod", Namespace: notManagedNamespaceName, Annotations: afterAllocationAnnotation(notManagedNamespaceName, unmanagedNamespaceMAC)}} | ||
| vmConfigMap := v1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: testManagerNamespace, Name: names.WAITING_VMS_CONFIGMAP}} |
There was a problem hiding this comment.
My personal note, I would break these long lines, its a bit a hard to read the initialization of the struct here.
|
@enp0s3: changing LGTM is restricted to collaborators In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
using formatted view instead of long lines improves readability Signed-off-by: Ram Lavi <ralavi@redhat.com>
| managedPodWithMacAllocated := v1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "podpod", | ||
| Namespace: managedNamespaceName, | ||
| Annotations: afterAllocationAnnotation(managedNamespaceName, managedNamespaceMAC), | ||
| }, | ||
| } | ||
|
|
||
| unmanagedPodWithMacAllocated := v1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "unmanagedPod", | ||
| Namespace: notManagedNamespaceName, | ||
| Annotations: afterAllocationAnnotation(notManagedNamespaceName, unmanagedNamespaceMAC), | ||
| }, | ||
| } | ||
|
|
||
| vmConfigMap := v1.ConfigMap{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: testManagerNamespace, | ||
| Name: names.WAITING_VMS_CONFIGMAP, | ||
| }, | ||
| } |
|
@RamLavi Thing is that I am not a member in k8s network plumbing org |
|
@ormergi can you take a look? |
| }, | ||
| Webhooks: []admissionregistrationv1.MutatingWebhook{ | ||
| admissionregistrationv1.MutatingWebhook{ | ||
| Name: virtualMachnesWebhookName, |
There was a problem hiding this comment.
virtualMachnesWebhookName typo (also in original code, so not mandatory of course)
There was a problem hiding this comment.
I will make a mental note to fix it on the next refactor PR
| @@ -293,7 +292,6 @@ var _ = Describe("Pool", func() { | |||
| err := poolManager.AllocateVirtualMachineMac(&newVM, &transactionTimestamp, true, logger) | |||
| Expect(err).ToNot(HaveOccurred()) | |||
|
|
|||
| Expect(poolManager.macPoolMap).To(HaveLen(2)) | |||
There was a problem hiding this comment.
Change is fine, i just wonder, isnt it better to be explicit ? (not sure, maybe robust is better indeed)
just food for thought
There was a problem hiding this comment.
It is still explicit, I just moved the check to the checkMacPoolMapEntries function, so that all tests benefit from it.
|
Thanks @oshoval ! /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RamLavi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
This PR does refactor chores on the pool-manager library:
Special notes for your reviewer:
Release note: