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
Vtop: Refactor Conditions to List for Kubectl Wait #136
Conversation
…ess. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
|
||
// GetCondition provides map style access to retrieve a condition from the conditions list by it's type | ||
// If the condition doesn't exist, we return nil, false. | ||
func (s *VitessKeyspaceStatus) GetCondition(ty VitessKeyspaceConditionType) (value *VitessKeyspaceCondition, exists bool) { |
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 it may be bug-prone to have an exported method that returns a pointer into the underlying array. Callers may think it's ok to retain the pointer, but that's not safe in general. Either the caller's value might get unintentionally mutated underneath them, or the caller might intend to mutate the value in the array and fail.
Consider this scenario:
ptr := GetCondition(condType)
SetConditionStatus(otherCondType, ...)
// This may or may not update the value in the array,
// depending on whether SetConditionStatus() above
// allocated a new array while appending to the slice.
ptr.Status = corev1.ConditionTrue
I think it'll be useful to have an exported GetCondition()
method, but it should return a deep copy to be safe. We could convert this version to an unexported method getCondition()
that's used by both SetConditionStatus()
and GetCondition()
.
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.
Ahh, I see what you're saying. Ok, will adjust.
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.
Adjusted.
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
…tion. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
…feedback. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
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.
LGTM
* Refactored conditions to be a list, and have methods for map like access. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * Oops. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * Update shard conditions per new list design for Wait support. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * List size needs to be 0 to use append. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * Refactored so public GetCondition returns deep copy per review suggestion. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * Remove left over fragment I missed. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * Reverted changes to VitessShardStatus and related methods per review feedback. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
* Refactored conditions to be a list, and have methods for map like access. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * Oops. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * Update shard conditions per new list design for Wait support. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * List size needs to be 0 to use append. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * Refactored so public GetCondition returns deep copy per review suggestion. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * Remove left over fragment I missed. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * Reverted changes to VitessShardStatus and related methods per review feedback. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
* Refactored conditions to be a list, and have methods for map like access. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * Oops. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * Update shard conditions per new list design for Wait support. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * List size needs to be 0 to use append. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * Refactored so public GetCondition returns deep copy per review suggestion. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * Remove left over fragment I missed. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com> * Reverted changes to VitessShardStatus and related methods per review feedback. Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>
This PR refactors conditions to be a list rather than map so they can be used with kubectl wait command.