Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

fix: CONTAINERD_VERSION unbound variable in VHD build script #583

Merged
merged 8 commits into from
Feb 26, 2019
Merged

fix: CONTAINERD_VERSION unbound variable in VHD build script #583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
af89b75
fix: CONTAINERD_VERSION unbound variable in VHD build script
jackfrancis Feb 26, 2019
b8ec0da
fix: CONTAINERD_DOWNLOAD_URL var doesn’t have to be global
jackfrancis Feb 26, 2019
4773fa8
chore: remove quotes for PRIVATE_AZURE_REGISTRY_SERVER
jackfrancis Feb 26, 2019
2e0fa23
fix: missing [[ ]]
jackfrancis Feb 26, 2019
f3f3af9
fix: let’s try this!
jackfrancis Feb 26, 2019
a3f9aff
chore: remove unnecessary CONTAINERD_DOWNLOAD_URL
jackfrancis Feb 26, 2019
fc56593
chore: move CONTAINERD_DOWNLOAD_URL_BASE into calling function
jackfrancis Feb 26, 2019
fe50e9c
chore: don’t define var multiple times
jackfrancis Feb 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 2 additions & 5 deletions packer/install-dependencies.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
#!/bin/bash

CONTAINERD_DOWNLOAD_URL_BASE="https://storage.googleapis.com/cri-containerd-release/"

source /home/packer/provision_installs.sh
source /home/packer/provision_source.sh

Expand Down Expand Up @@ -40,8 +37,8 @@ for CNI_PLUGIN_VERSION in $CNI_PLUGIN_VERSIONS; do
downloadCNI
done

for CONTAINERD_VERSION in $CONTAINERD_VERSIONS; do
CONTAINERD_DOWNLOAD_URL="${CONTAINERD_DOWNLOAD_URL_BASE}cri-containerd-${CONTAINERD_VERSION}.linux-amd64.tar.gz"
CONTAINERD_DOWNLOAD_URL_BASE="https://storage.googleapis.com/cri-containerd-release/"
for CONTAINERD_VERSION in ${CONTAINERD_VERSIONS}; do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm being picky but the var assignment doesn't need to be inside the for loop since it's static

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

downloadContainerd
done

Expand Down
6 changes: 3 additions & 3 deletions parts/k8s/kubernetesinstalls.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ CNI_CONFIG_DIR="/etc/cni/net.d"
CNI_BIN_DIR="/opt/cni/bin"
CNI_DOWNLOADS_DIR="/opt/cni/downloads"
CONTAINERD_DOWNLOADS_DIR="/opt/containerd/downloads"
CONTAINERD_DOWNLOAD_URL="${CONTAINERD_DOWNLOAD_URL_BASE}cri-containerd-${CONTAINERD_VERSION}.linux-amd64.tar.gz"

removeEtcd() {
rm -rf /usr/bin/etcd
Expand Down Expand Up @@ -187,6 +186,7 @@ downloadAzureCNI() {
}

downloadContainerd() {
CONTAINERD_DOWNLOAD_URL="${CONTAINERD_DOWNLOAD_URL_BASE}cri-containerd-${CONTAINERD_VERSION}.linux-amd64.tar.gz"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're already setting CONTAINERD_DOWNLOAD_URL in the VHD script itself so this will override it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no longer setting CONTAINERD_DOWNLOAD_URL in the VHD script

mkdir -p $CONTAINERD_DOWNLOADS_DIR
CONTAINERD_TGZ_TMP=$(echo ${CONTAINERD_DOWNLOAD_URL} | cut -d "/" -f 5)
retrycmd_get_tarball 120 5 "$CONTAINERD_DOWNLOADS_DIR/${CONTAINERD_TGZ_TMP}" ${CONTAINERD_DOWNLOAD_URL} || exit $ERR_CONTAINERD_DOWNLOAD_TIMEOUT
Expand Down Expand Up @@ -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"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (1) removes the need for a 2nd concatenation of the $CONTAINERD_DOWNLOAD_URL var, and (2) is arguably more maintainable

rm -Rf /usr/bin/containerd
rm -Rf /var/lib/docker/containerd
rm -Rf /run/docker/containerd
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double [[ and ]] just because it's more idiomatic w/ the rest of the bash surface area

$CLI_TOOL login -u $SERVICE_PRINCIPAL_CLIENT_ID -p $SERVICE_PRINCIPAL_CLIENT_SECRET $PRIVATE_AZURE_REGISTRY_SERVER
fi
retrycmd_if_failure 60 1 1200 $CLI_TOOL pull $DOCKER_IMAGE_URL || exit $ERR_CONTAINER_IMG_PULL_TIMEOUT
Expand Down