enhanced script to be able to support waiting for stateful sets#15
enhanced script to be able to support waiting for stateful sets#15christiangroth wants to merge 3 commits intotimoreimann:masterfrom christiangroth:master
Conversation
timoreimann
left a comment
timoreimann
left a comment
There was a problem hiding this comment.
Thanks for your PR, would definitely be good to have it!
I made one suggestion, let me know what you think.
wait-for-deployment
Outdated
|
|
||
| display_usage_and_exit() { | ||
| echo "Usage: $(basename "$0") [-n <namespace>] [-t <timeout>] <deployment>" >&2 | ||
| echo "Usage: $(basename "$0") [-n <namespace>] [-t <timeout>] <deployment|statefulset> <resource-name>" >&2 |
There was a problem hiding this comment.
Instead of requiring another parameter that would break existing users, how about expecting callers to define the type as a slash-delimited prefix to the resource name like
[<resource type>/]<resource name>
where <resource type> is optional and defaults to deployment. This would also be in line with what you see in k8s land / kubectl.
There was a problem hiding this comment.
I like this idea. Unfortunately I don't have time to implement and test it in the moment, it will take a while.
There was a problem hiding this comment.
No hurry on my end (how could I expect that anyway ;) ), take your time!
There was a problem hiding this comment.
I've pushed a new commit, changing the script back to using one commandline argument only. This should do it :)
timoreimann
left a comment
timoreimann
left a comment
There was a problem hiding this comment.
Some small comments, but looking good already.
wait-for-deployment
Outdated
| echo "Usage: $(basename "$0") [-n <namespace>] [-t <timeout>] [deployment|statefulset]/<resource-name>" >&2 | ||
| echo "Arguments:" >&2 | ||
| echo "deployment REQUIRED: The name of the deployment the script should wait on" >&2 | ||
| echo "[deployment/statefulset]/resource-name REQUIRED: The type and name of the resource the script should wait on. Currently the supported types are deployment and statefulset are supported." >&2 |
There was a problem hiding this comment.
One mentioning of the types being supported too much :)
| if [ "$#" -ne 1 ] ; then | ||
| display_usage_and_exit | ||
| fi | ||
| readonly deployment="$1" |
There was a problem hiding this comment.
Could you re-apply the readonly declaration after line 92 (after which we do not need to mutate the variable again AFAIU)?
| get_available_replicas() { | ||
| get_deployment_jsonpath '{.status.availableReplicas}' | ||
| get_ready_replicas() { | ||
| get_deployment_jsonpath '{.status.readyReplicas}' |
There was a problem hiding this comment.
Can you explain why you changed this from available to ready? Apologies, it's been a while since I looked into the exact logic.
There was a problem hiding this comment.
It's because readyReplicas is available for both deployment and stateful sets and availableReplicas only for deployments. So using readyReplicas allows us checking deployments and stateful sets the same way.
2 participants
Hi,
I've enhanced the script to be able to support waiting for stateful sets also. Maybe you want to get this changes back.
Chris