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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Persistent Volumes #1284
Add support for Persistent Volumes #1284
Changes from 2 commits
1b49c6f
4582cfd
98ab07c
69a9ee9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think will be helpful to have a function that can get status as argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😊