General improvements
The following discussions from !4 (merged) should be addressed:
-
@alossent started a discussion: (+2 comments) Shouldn't this be the very first check to do? There is nothing to do if the phase is not
Released
, so it seems strange to do all the above computations when you could start the loop withif persV.Status.Phase != "Released" { // nothing to do continue }
-
@alossent started a discussion: (+1 comment) A general comment:
The move of the volume reclaiming configuration to annotations on PVs is an opportunity to simply the logic of this component: in particular the storageclass does not matter anymore (that was the point of the change), however it's still there all over the place (there's a flag to specify a storageClass, but it doesn't seem we need that anymore?).
Comments really need more attention. I only reported the worst inconsistencies, more of them are out of sync with the code. Keep in mind that there is no need to paraphrase the code, the most useful comments rather explain why things are done the way they are done (explain the design choices, the high-level logic etc.). Well-chosen, explicit variable naming helps to reduce the need for comments by making the code easier to understand (vague names like
tFuture
could be improved, for instance).Also consider writing more helper functions to factorize tasks such as checking if an annotation exists and is a valid duration (or date), which is done in multiple places. This could help streamlining the big loop.
There is also an opportunity to put your growing experience manipulating dates to the work here and follow similar patterns as the backup stats when storing dates in annotations.