-
Notifications
You must be signed in to change notification settings - Fork 114
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
Support development version of Kustomize in install script #428
Conversation
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.
Hi @janwytze, thanks for the PR. Along with testing the load restrictor logic below, other scripts would need to include your update as well e.g. /scripts/setup_user_namespaces.sh and /scripts/delete.sh.
scripts/install.sh
Outdated
@@ -378,7 +378,7 @@ wait_for_pods_ready "-l control-plane=modelmesh-controller" | |||
|
|||
# Older versions of kustomize have different load restrictor flag formats. | |||
# Can be removed once Kubeflow installation stops requiring v3.2. | |||
kustomize_version=$(kustomize version --short | grep -o -E "[0-9]\.[0-9]\.[0-9]") | |||
kustomize_version=$(kustomize version --short | grep -o -E "[0-9]\.[0-9]\.[0-9]|\(devel\)") |
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.
Can you confirm that the logic below for the kustomize_load_restrictor_arg
continues to work with the change? Seems to me like with kustomize version (devel)
, I hit Error: unknown flag: --load_restrictor none
.
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.
@rafvasq The output of kustomize version --short
on my machine is:
Flag --short has been deprecated, and will be removed in the future.
{(devel) unknown }
This causes $kustomize_load_restrictor_arg
to be set to:
--load-restrictor LoadRestrictionsNone
I believe this is the correct behavior. Both the install
and delete
scripts succeed for me after this PR.
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.
My output is
Flag --short has been deprecated, and will be removed in the future.
{(devel) 2023-09-11T18:05:37Z }
and results in $kustomize_load_restrictor_arg
to be --load_restrictor none
.
I'm not sure if it's a variation in how string comparisons are handled across Bash versions, locale settings, or something else, but regardless it I think the logic could be hardened.
scripts/install.sh
Outdated
@@ -378,7 +378,7 @@ wait_for_pods_ready "-l control-plane=modelmesh-controller" | |||
|
|||
# Older versions of kustomize have different load restrictor flag formats. | |||
# Can be removed once Kubeflow installation stops requiring v3.2. | |||
kustomize_version=$(kustomize version --short | grep -o -E "[0-9]\.[0-9]\.[0-9]") | |||
kustomize_version=$(kustomize version --short | grep -o -E "[0-9]\.[0-9]\.[0-9]|\(devel\)") |
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.
My output is
Flag --short has been deprecated, and will be removed in the future.
{(devel) 2023-09-11T18:05:37Z }
and results in $kustomize_load_restrictor_arg
to be --load_restrictor none
.
I'm not sure if it's a variation in how string comparisons are handled across Bash versions, locale settings, or something else, but regardless it I think the logic could be hardened.
This comment was marked as duplicate.
This comment was marked as duplicate.
I removed my earlier review after doing a bit more testing. Will update shortly. |
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.
Instead of trying to capture the kustomize
CLI version, we could check for the spelling and parameter value variation of the supported load restrictor flag
load_restrictor_flag=$( kustomize build --help | grep -o -E "\-\-load.restrictor[^,]+" | sed -E "s/(--load.restrictor).+'(.*none)'/\1 \2/I" )
...
kustomize build runtimes $load_restrictor_flag
Quick test:
# shell function to set kustomize version, download CLI to user local bin if it doesn't exists
set-kustomize-version() {
# check and process function parameters
[ -z "${1}" ] && echo "Usage: '${FUNCNAME[0]} <version>'" && return 1
[[ ! "$1" =~ ^[0-9]+(\.[0-9]+){2}$ ]] && echo "<version> should be of format 'x.y.z'" && return 1
VERSION="$1"
# https://kubectl.docs.kubernetes.io/installation/kustomize/binaries/
INSTALL_SCRIPT_URL="https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh"
# create user-local kustomize install dir
INSTALL_DIR=~/bin/kustomize/${VERSION}
mkdir -p ${INSTALL_DIR}
# download the requested kustomize CLI, if not done before
[ -f ${INSTALL_DIR}/kustomize ] || curl -s ${INSTALL_SCRIPT_URL} | bash -s -- ${VERSION} ${INSTALL_DIR}
# prepend the session PATH with the folder of the desired kustomize CLI version
export PATH=${INSTALL_DIR}:$PATH
# report the set kustomize CLI version to the user
echo -n "Kustomize version: "
(kustomize version --short 2> /dev/null || kustomize version) | grep -o -E "[0-9]\.[0-9]\.[0-9]"
}
# specify key kustomize versions
kustomize_versions=("3.3.0" "4.0.0" "4.1.0")
# navigate to the "config" folder inside modelmesh-serving repo
cd config
# for each of the kustomize versions, try to build the runtimes with the correct load-restrictor flag
for v in ${kustomize_versions[@]}; do
set-kustomize-version $v
load_restrictor_flag=$( kustomize build --help | grep -o -E "\-\-load.restrictor[^,]+" | sed -E "s/(--load.restrictor).+'(.*none)'/\1 \2/I" )
echo "$load_restrictor_flag"
kustomize build runtimes $load_restrictor_flag 2>&1 > /dev/null \
&& echo "SUCCESS" \
|| echo "FAILED"
echo
done
Output on MacOS and Linux:
Kustomize version: 3.3.0
--load_restrictor none
SUCCESS
Kustomize version: 4.0.0
--load_restrictor LoadRestrictionsNone
SUCCESS
Kustomize version: 4.1.0
--load-restrictor LoadRestrictionsNone
SUCCESS
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.
Tested with versions 3.2.0
, 5.1.1
, and (devel)
. Thanks @janwytze!
setup_user_namespaces scripts Signed-off-by: Jan Wytze Zuidema <[email protected]>
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.
/lgtm
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ckadner, janwytze, rafvasq The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Closes #427
Motivation
Kustomize version
(devel)
is not supported by the install script.Modifications
Changed the regex so
(devel)
is a supported version.Result
Install script now works when Kustomize is installed from https://kubectl.docs.kubernetes.io/installation/kustomize/source/