Add support for Persistent Volumes #1284
Conversation
modules/k8s/persistent_volume.go
Outdated
|
|
||
| // IsPersistentVolume returns true if the given PersistentVolume is available | ||
| func IsPersistentVolumeAvailable(pv *corev1.PersistentVolume) bool { | ||
| return pv != nil && pv.Status.Phase == corev1.VolumeAvailable |
There was a problem hiding this comment.
I was wondering if should be checked for corev1.VolumeBound / corev1.VolumeReleased?
Can be use-cases when is required to check already bound PV
There was a problem hiding this comment.
My initial intention was to have a generic WaitForPersistentVolumeInPhase which would receive the Phase to wait for. Then I would have smaller wrapper functions for each state.
However, at the end, and due to our requirements, I just implemented the 'Available' one, with the idea of implementing the others when I needed them.
I could implement the whole thing now though
There was a problem hiding this comment.
FWIW, the next PR, which will be for PersistentVolumeClaims, has a function which waits for the bound state
There was a problem hiding this comment.
Thanks for the review @denis256 , I'll address the comments. What about this one? Is it enough with what I have or you prefer me to implement other checks?
There was a problem hiding this comment.
I think will be helpful to have a function that can get status as argument
There was a problem hiding this comment.
Done.
I replaced the wait and test functions to check the 'available' status for generic ones which receive the status phase. I've named the status argument pvStatusPhase to make clear the relationship between the status and the type of argument the functions receive (corev1.PersistentVolumePhase, which is what's compared).
I think there's no need to add smaller wrapper functions for each phase since the generic functions are clear enough. Adding the functions would make the module unnecessarily long since there're 5 possible status phases (which means 10 extra functions).
Waiting for you comments 😊
|
Hi! I think that the failed tests are not related to my changes, are they? Is there anything else I need to fix that I missed? |
|
Failing tests aren't related to implemented changes |
kaisoz
force-pushed
the
add-k8s-persistent-volume-support
branch
from
2b1b965
to
98ab07c
Compare
Description
This PR adds support for
PersistentVolumesas stated in #1283TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Migration Guide
There aren`t backward incompatible changes