fix: CONTAINERD_VERSION unbound variable in VHD build script #583
Conversation
| @@ -220,7 +220,7 @@ installContainerd() { | |||
| if [[ "$CURRENT_VERSION" == "${CONTAINERD_VERSION}" ]]; then | |||
| echo "containerd is already installed, skipping install" | |||
| else | |||
| CONTAINERD_TGZ_TMP=$(echo ${CONTAINERD_DOWNLOAD_URL} | cut -d "/" -f 5) | |||
| CONTAINERD_TGZ_TMP="cri-containerd-${CONTAINERD_VERSION}.linux-amd64.tar.gz" | |||
There was a problem hiding this comment.
This (1) removes the need for a 2nd concatenation of the $CONTAINERD_DOWNLOAD_URL var, and (2) is arguably more maintainable
| @@ -187,6 +186,7 @@ downloadAzureCNI() { | |||
| } | |||
|
|
|||
| downloadContainerd() { | |||
| CONTAINERD_DOWNLOAD_URL="${CONTAINERD_DOWNLOAD_URL_BASE}cri-containerd-${CONTAINERD_VERSION}.linux-amd64.tar.gz" | |||
There was a problem hiding this comment.
we're already setting CONTAINERD_DOWNLOAD_URL in the VHD script itself so this will override it
There was a problem hiding this comment.
no longer setting CONTAINERD_DOWNLOAD_URL in the VHD script
|
If that helps I have a passing vhd build with https://github.com/Azure/aks-engine/pull/457/files |
Codecov Report
@@ Coverage Diff @@
## master #583 +/- ##
=======================================
Coverage 56.78% 56.78%
=======================================
Files 91 91
Lines 13894 13894
=======================================
Hits 7890 7890
Misses 5338 5338
Partials 666 666 |
| @@ -40,7 +40,7 @@ for CNI_PLUGIN_VERSION in $CNI_PLUGIN_VERSIONS; do | |||
| downloadCNI | |||
| done | |||
|
|
|||
| for CONTAINERD_VERSION in $CONTAINERD_VERSIONS; do | |||
| for CONTAINERD_VERSION in ${CONTAINERD_VERSIONS}; do | |||
There was a problem hiding this comment.
can we move CONTAINERD_DOWNLOAD_URL_BASE down here now that it's not used until downloadContainerd is called? It had to be at the top of the file before because CONTAINERD_DOWNLOAD_URL was set at the top of the sourced file
There was a problem hiding this comment.
I'm being picky but the var assignment doesn't need to be inside the for loop since it's static
| @@ -278,7 +278,7 @@ installKubeletAndKubectl() { | |||
| pullContainerImage() { | |||
| CLI_TOOL=$1 | |||
| DOCKER_IMAGE_URL=$2 | |||
| if [ ! -z "${PRIVATE_AZURE_REGISTRY_SERVER}" ]; then | |||
| if [[ ! -z "${PRIVATE_AZURE_REGISTRY_SERVER:-}" ]]; then | |||
There was a problem hiding this comment.
double [[ and ]] just because it's more idiomatic w/ the rest of the bash surface area
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon, jackfrancis The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Reason for Change:
The
$CONTAINERD_DOWNLOAD_URLwas present as a global variable in the base k8s CSE install script, and depended upon$CONTAINERD_VERSIONto construct itself. This removes$CONTAINERD_DOWNLOAD_URLfrom the global variable namespace to allow the CSE script to once again be generally sourced by another script (without having to pass a$CONTAINERD_VERSIONenv var).Issue Fixed:
Requirements:
Notes: