implement K8S_TEST_NAMESPACE #20
Conversation
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
skylib/k8s_test_namespace.sh.tpl
Outdated
| set +e | ||
| if [ -n "${K8S_MYNAMESPACE:-}" ] | ||
| if [ -n "${K8S_TEST_NAMESPACE:-}" ] | ||
| then | ||
| # use provided namespace | ||
| NAMESPACE=${K8S_TEST_NAMESPACE} | ||
| # do not delete namespace after the test is complete | ||
| DELETE_NAMESPACE_FLAG="" | ||
| elif [ -n "${K8S_MYNAMESPACE:-}" ] | ||
| then | ||
| # do not create random namesspace | ||
| NAMESPACE=`whoami` |
There was a problem hiding this comment.
The script uses set +e which disables the automatic exit when a command fails. This can lead to the script continuing execution even after critical commands fail, potentially leading to unexpected behavior or security issues. It's recommended to handle errors explicitly and use set -e to ensure the script exits on failure.
The use of backticks for command substitution in line 58 is deprecated in favor of $(...) syntax. The latter is more readable, especially in complex commands or when nested. It's recommended to replace backticks with $(whoami) for better readability and maintainability.
| set +e | ||
| if [ -n "${K8S_MYNAMESPACE:-}" ] | ||
| if [ -n "${K8S_TEST_NAMESPACE:-}" ] | ||
| then | ||
| # use provided namespace | ||
| NAMESPACE=${K8S_TEST_NAMESPACE} | ||
| # do not delete namespace after the test is complete | ||
| DELETE_NAMESPACE_FLAG="" | ||
| elif [ -n "${K8S_MYNAMESPACE:-}" ] | ||
| then | ||
| # do not create random namesspace | ||
| NAMESPACE=`whoami` | ||
| NAMESPACE=$(whoami) | ||
| # do not delete namespace after the test is complete | ||
| DELETE_NAMESPACE_FLAG="" | ||
| else |
There was a problem hiding this comment.
The script's logic for determining the namespace and whether to delete it after tests could lead to unintended consequences due to its reliance on environment variables without clear validation or fallback. Specifically, the script assumes that if K8S_TEST_NAMESPACE or K8S_MYNAMESPACE is set, these should be used without further checks. This could potentially lead to using incorrect namespaces or not deleting namespaces when expected, especially in complex CI environments where environment variables might be set unintentionally.
To improve this, introduce explicit validation for the namespace values being used, ensuring they conform to expected patterns or naming conventions. Additionally, consider adding a clear, documented default behavior for namespace creation and deletion when neither environment variable is set, to avoid ambiguity and ensure clean-up is handled correctly in all scenarios. This approach will enhance the script's robustness and predictability, making it safer to use in diverse environments.
# This is the 1st commit message: app name first # This is the commit message nazzzzz#2: clean up file # This is the commit message nazzzzz#3: fix reference to proto package # This is the commit message fasterci#4: better parse # This is the commit message fasterci#5: fix again # This is the commit message fasterci#6: fix again # This is the commit message fasterci#7: fix again # This is the commit message fasterci#8: fix again # This is the commit message fasterci#9: fix again # This is the commit message fasterci#10: fix again # This is the commit message fasterci#11: add debug statements for seeing if branch is dirty # This is the commit message fasterci#12: fix again # This is the commit message fasterci#13: fix again # This is the commit message fasterci#14: fix again # This is the commit message fasterci#15: fix again # This is the commit message fasterci#16: fix again # This is the commit message fasterci#17: fix again # This is the commit message fasterci#18: fix again # This is the commit message fasterci#19: fix again # This is the commit message fasterci#20: fix again # This is the commit message fasterci#21: fix again # This is the commit message fasterci#22: fix again # This is the commit message fasterci#23: fix again
implement K8S_TEST_NAMESPACE to allow to run integration test in specified namespace.
This is a backwards-compatible reimplementation of adobe/rules_gitops@3159eb6