-
Notifications
You must be signed in to change notification settings - Fork 25
Improve ubuntu upgrade bash scripts #2142
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
base: stackhpc/2024.1
Are you sure you want to change the base?
Conversation
- Fix checking of undefined variables - More quoting
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.
Code Review
This pull request improves the robustness of the Ubuntu upgrade scripts by fixing checks for undefined variables and adding quoting, which are great improvements. The changes correctly address the issues described.
I've added a couple of suggestions on tools/ubuntu-upgrade-infra-vm.sh to make the scripts even more concise and maintainable by using more idiomatic bash constructs. These suggestions also apply to the other scripts (ubuntu-upgrade-overcloud.sh, ubuntu-upgrade-seed-hypervisor.sh, ubuntu-upgrade-seed.sh) as they share similar code blocks. Applying these changes across all scripts would reduce code duplication and improve overall consistency.
| if [[ -z "$KAYOBE_PATH" ]]; then | ||
| echo "Environment variable \$KAYOBE_PATH is not defined" | ||
| exit 2 | ||
| fi | ||
|
|
||
| if [[ ! $KAYOBE_CONFIG_PATH ]]; then | ||
| if [[ -z "$KAYOBE_CONFIG_PATH" ]]; then | ||
| echo "Environment variable \$KAYOBE_CONFIG_PATH is not defined" | ||
| exit 2 | ||
| fi |
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.
For checking required environment variables, you can use parameter expansion to make the code more concise and idiomatic. This will check if the variable is set and not empty, and if not, it will print the error message to stderr and exit the script due to set -e. This pattern can be applied to the other scripts as well.
| if [[ -z "$KAYOBE_PATH" ]]; then | |
| echo "Environment variable \$KAYOBE_PATH is not defined" | |
| exit 2 | |
| fi | |
| if [[ ! $KAYOBE_CONFIG_PATH ]]; then | |
| if [[ -z "$KAYOBE_CONFIG_PATH" ]]; then | |
| echo "Environment variable \$KAYOBE_CONFIG_PATH is not defined" | |
| exit 2 | |
| fi | |
| : "${KAYOBE_PATH:?Environment variable KAYOBE_PATH is not defined}" | |
| : "${KAYOBE_CONFIG_PATH:?Environment variable KAYOBE_CONFIG_PATH is not defined}" |
| if [[ -z "$ANSIBLE_ROLES_PATH" ]]; then | ||
| set -x | ||
| export ANSIBLE_ROLES_PATH=$KAYOBE_PATH/ansible/roles | ||
| export ANSIBLE_ROLES_PATH="$KAYOBE_PATH/ansible/roles" | ||
| set +x | ||
| else | ||
| set -x | ||
| export ANSIBLE_ROLES_PATH=$ANSIBLE_ROLES_PATH:$KAYOBE_PATH/ansible/roles | ||
| export ANSIBLE_ROLES_PATH="$ANSIBLE_ROLES_PATH:$KAYOBE_PATH/ansible/roles" | ||
| set +x | ||
| fi |
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.
This logic for setting ANSIBLE_ROLES_PATH can be simplified to be less repetitive. By preparing the new value in a temporary variable, you can avoid duplicating the set -x/export/set +x block. This refactoring improves readability and maintainability, and can be applied to the other scripts as well.
| if [[ -z "$ANSIBLE_ROLES_PATH" ]]; then | |
| set -x | |
| export ANSIBLE_ROLES_PATH=$KAYOBE_PATH/ansible/roles | |
| export ANSIBLE_ROLES_PATH="$KAYOBE_PATH/ansible/roles" | |
| set +x | |
| else | |
| set -x | |
| export ANSIBLE_ROLES_PATH=$ANSIBLE_ROLES_PATH:$KAYOBE_PATH/ansible/roles | |
| export ANSIBLE_ROLES_PATH="$ANSIBLE_ROLES_PATH:$KAYOBE_PATH/ansible/roles" | |
| set +x | |
| fi | |
| if [[ -z "$ANSIBLE_ROLES_PATH" ]]; then | |
| ANSIBLE_ROLES_PATH_NEW="$KAYOBE_PATH/ansible/roles" | |
| else | |
| ANSIBLE_ROLES_PATH_NEW="$ANSIBLE_ROLES_PATH:$KAYOBE_PATH/ansible/roles" | |
| fi | |
| set -x | |
| export ANSIBLE_ROLES_PATH="$ANSIBLE_ROLES_PATH_NEW" | |
| set +x |
Concrete issue was that it failed to correctly check KAYOBE_PATH was set.