search

kyma-incubator / local-kyma

Local installation on k3d cluster
14 stars 10 forks source link

Add shell-script options #14

Closed kleino closed 4 years ago

kleino commented 4 years ago

Hi, thanks a lot for creating this setup-script for k3d, it has been quite helpful already :)

One minor improvement suggestion: In its current form it could be potentially quite dangerous to use due to the missing error handling. If one of the commands fails, the other commands will still be executed. This can have unintended consequences if e.g. k3d cluster ... fails and the k8s-context is not updated. In that case, all the following helm & kubectl would be executed for the current which is probably not a good idea :D set -o errexit fixes that by aborting the script in case any of the commands fail. Further reading: http://www.yoone.eu/articles/2-good-practices-for-writing-shell-scripts.html

best wishes :)

CLAassistant commented 4 years ago

CLA assistant check
All committers have signed the CLA.

kleino commented 4 years ago

Makes sense of course, I didnt really check that in this case. Script errors can be ignored by adding something like || true or || echo "completing anyway", e.g.

#script will continue even if following command fails.
docker network create k3d-kyma || echo "Network already exists"
pbochynski commented 4 years ago

The PR is obsolete know, as the script got a lot of refactoring. Cluster creation is separated now from installation. Thanks for the PR - your advice will be applied in other scripts.