contrib/script: To add script for kind cluster #13797
Conversation
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
maintainer-s-little-helper
bot
assigned michi-covalent and qmonnet and unassigned michi-covalent and qmonnet
sayboras
force-pushed
the
contrib/kind-script
branch
4 times, most recently
from
ae2b4f6
to
b3d4893
Compare
sayboras
added
area/misc
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
There was a problem hiding this comment.
I don't have much experience with kind, but the script looks good overall, thanks!
Please find a few issues to fix and some suggestions below.
One other thing, should the script be mentioned in the documentation maybe?
I leave it to you and Chris to see what version of the script is preferable.
| --set operator.prometheus.enabled=true \ | ||
| --set hubble.enabled=true \ | ||
| --set hubble.metrics.enabled="{dns,drop,tcp,flow,port-distribution,icmp,http}" | ||
| fi |
There was a problem hiding this comment.
Just one suggestion: I would be tempted to have a set of options common to IPv4 and IPv6, and to complete with just the relevant ones in the if... else... fi. It would avoid duplicating a good portion of those options? But I'm fine if you prefer to keep this version, too.
| # ./contrib/scripts/kind.sh | ||
| # IPv6=1 ./contrib/scripts/kind.sh | ||
| # NO_BUILD=1 ./contrib/scripts/kind.sh | ||
| # NO_PROVISION=1 ./contrib/scripts/kind.sh |
There was a problem hiding this comment.
You could check for the number of arguments passed to the script, and print a help message shortly describing those variables if the user tries to pass any argument (which would likely be -h or --help if they haven't opened the script to read it).
sayboras
force-pushed
the
contrib/kind-script
branch
from
b3d4893
to
57b8364
Compare
|
@qmonnet thanks for your review, I have corrected silly mistake from my side. I will wait for Chris input first before addressing your pending comments, hope its fine :) |
|
Awesome and sure, no problem! |
There was a problem hiding this comment.
Nice! This is definitely useful to automate for GH Actions.
My script in #12527 was mostly intended for local use. That is why I also included a way to deploy a local registry into the kind cluster for quick iteration while developing because pushing images externally can be slow. Some of my comments are based on this. Do you plan for this script to be used for local dev as well? If so, it'd be nice to have a local registry like my PR adds (you can probably just rip it out from there.)
It would be good to also document this as well.
No need for you to close this PR; this is much further along π
| # Set NO_BUILD=1 to avoid building docker images again | ||
| export 'NO_BUILD'="${NO_BUILD:0}" | ||
| # Set NO_PROVISION=1 to use existing kind cluster | ||
| export 'NO_PROVISION'="${NO_PROVISION:0}" |
There was a problem hiding this comment.
I find it easier to deal with flags if they are in the "positive" tone. Meaning, we can have a BUILD flag that's by default 1 and the user would need to say BUILD=0 ... to disable the building, instead of enabling the "negative".
I know we have precedence for using the "negative" tone in our Vagrantfiles, but maybe we can start to move away from that pattern. What do you think?
| if [[ "${NO_PROVISION}" -ne "1" ]]; then | ||
| kind delete cluster | ||
| # clean up previous kind network | ||
| docker system prune -f |
There was a problem hiding this comment.
If this script is intended to be used locally by users wanting to spin up a local kind cluster, then this line will remove a lot more things than just the previous kind network, e.g. build cache.
| kind load docker-image cilium/cilium:latest | ||
| kind load docker-image cilium/operator-generic:latest |
There was a problem hiding this comment.
If this script is intended for local use, then the user might want to deploy their own dev images.
|
Thanks both for your review comments, let me mark this one as draft to reducing unnecessary notifications while I am incorporating your comments. Will mark it ready once done :) |
This commit is to add the script to bring up kind cluster with cilium. Most of the options and steps are similar as smoke-test in github action. Hence, it will help to some capacity to debug any failure in smoke test as well. Signed-off-by: Tam Mach <sayboras@yahoo.com>
sayboras
force-pushed
the
contrib/kind-script
branch
from
57b8364
to
b19d9f2
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
stale
bot
added
the
stale
|
This issue has not seen any activity since it was marked stale. Closing. |
This commit is to add the script to bring up kind cluster with cilium.
Most of the options and steps are similar as smoke-test in github
action. Hence, it will help to some capacity to debug any failure in
smoke test as well.
Signed-off-by: Tam Mach sayboras@yahoo.com
PS: Sorry, I just remembered that you have similar PR #12527 after
created this, I can close this one as well /cc @christarazi
Testing
I used this one for quite sometimes as part of local development, some sample run can be found as below
NO_BUILD=1 IPv6=1 ./contrib/scripts/kind.sh
NO_BUILD=1 ./contrib/scripts/kind.sh
NO_BUILD=1 NO_PROVISION=1 ./contrib/scripts/kind.sh