Add extraMounts functional tests #476
Conversation
|
/retest |
1 similar comment
|
/retest |
| ss := th.GetStatefulSet(volume) | ||
| // Check the resulting deployment fields | ||
| Expect(int(*ss.Spec.Replicas)).To(Equal(1)) | ||
| Expect(ss.Spec.Template.Spec.Volumes).To(HaveLen(15)) |
There was a problem hiding this comment.
could also use https://github.com/openstack-k8s-operators/lib-common/blob/main/modules/common/test/helpers/volumes.go#L27 . to check that the volume with the expected name exist.
There was a problem hiding this comment.
Yes, I totally agree to this. Thank you, I wasn't aware about lib-common providing such assertion, and I'm going to use it. In general I don't like much checking the Volumes (or VolumeMount) array length by hardcoding a value, and the assertion solves this problem providing a more solid check!
| container := ss.Spec.Template.Spec.Containers[1] | ||
| // Fail if manila-share doesn't have the right number of | ||
| // VolumeMounts entries | ||
| Expect(container.VolumeMounts).To(HaveLen(17)) |
There was a problem hiding this comment.
we have https://github.com/openstack-k8s-operators/lib-common/blob/main/modules/common/test/helpers/volumes.go#L36 to check that an expected VolumeMount exist, but it does not (yet) provide functionality to validate the MountPath. should we enhance it?
There was a problem hiding this comment.
This seems a good idea as well. I was planning, as part of the other comment, to add a validation as part of the extraMount.propagation, to make sure the VolumeMount exists in the resulting statefulset, but perhaps when you mentioned validate the MountPath you had something different in mind (e.g. reserved paths or other relevant RFCs)
There was a problem hiding this comment.
in your previous version you were validating the volumemount path like
Expect(vm.MountPath).To(ContainSubstring(CinderCephExtraMountsPath))
we could enhance the test helpers to be able to do that. thats not something it checks.
There was a problem hiding this comment.
oh I see sorry. Even for MountPath we currently check the name and it would be good to be able to validate the path that way. I will add it to the existing assertion in lib-common, so we get it here with the next bump.
Thank you very much for the review, it's very useful and I wasn't aware about existing assertions provided for basic components in lib-common. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fmount The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This change adds a functional test for Cinder extraMounts. It ensures we're able to validate the abstraction of 'corev1.VolumeSource' struct introduced in lib-common/storage module. Jira: https://issues.redhat.com/browse/OSPRH-11210 Signed-off-by: Francesco Pantano <[email protected]>
|
/lgtm |
|
/test cinder-operator-build-deploy-kuttl |
1 similar comment
|
/test cinder-operator-build-deploy-kuttl |
This change adds a functional test for
CinderextraMounts. It ensures we're able to validate the abstraction ofcorev1.VolumeSourcestruct introduced inlib-common/storagemodule.Jira: https://issues.redhat.com/browse/OSPRH-11210