-
Notifications
You must be signed in to change notification settings - Fork 29
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
enhanced script to be able to support waiting for stateful sets #15
base: master
Are you sure you want to change the base?
Conversation
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No hurry on my end (how could I expect that anyway ;) ), take your time!
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've pushed a new commit, changing the script back to using one commandline argument only. This should do it :)
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.
Some small comments, but looking good already.
wait-for-deployment
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One mentioning of the types being supported too much :)
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.
Fixed :)
display_usage_and_exit | ||
fi | ||
readonly deployment="$1" |
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.
Could you re-apply the readonly declaration after line 92 (after which we do not need to mutate the variable again AFAIU)?
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.
Fixed :)
get_available_replicas() { | ||
get_deployment_jsonpath '{.status.availableReplicas}' | ||
get_ready_replicas() { | ||
get_deployment_jsonpath '{.status.readyReplicas}' |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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