-
Notifications
You must be signed in to change notification settings - Fork 62
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
add Kustomize 5.2.1 in Kind GHA testing #43
add Kustomize 5.2.1 in Kind GHA testing #43
Conversation
Signed-off-by: Matteo Mortari <[email protected]>
39ccb39
to
8dea997
Compare
Signed-off-by: Matteo Mortari <[email protected]>
8dea997
to
1235c35
Compare
Signed-off-by: Matteo Mortari <[email protected]>
18ed6b6
to
36a97b2
Compare
Signed-off-by: Matteo Mortari <[email protected]>
36a97b2
to
a664ac1
Compare
@@ -51,6 +50,7 @@ jobs: | |||
if: env.BUILD_CONTEXT == 'main' | |||
shell: bash | |||
env: | |||
IMG: ${{ env.IMG_ORG }}/${{ env.IMG_REPO }} |
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.
I'm probably missing something, but what's the reason for moving the IMG env variable?
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.
env at the top level of GHA cannot depend on one-another :(
so this was the original strategy by @lampajr and I'm just putting it back in place
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.
env at the top level of GHA cannot depend on one-anothe
+1 you cannot define top-level env variables from other ones, that's is only possible at step
level
run: | | ||
echo "Download kustomize 5.2.1" | ||
mkdir $GITHUB_WORKSPACE/kustomize | ||
curl -s "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh" | bash -s "5.2.1" "$GITHUB_WORKSPACE/kustomize" |
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.
Same question as other PR. Can we try to avoid using curl | bash?
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.
I tried first an alternative with
471563b#diff-ee244f6eeb2d7ac8c604cdc91588b49204dea5594df11b20af816ad9f4b54467R36-R39
that is using a dedicated GHA to download kustomize, but the performance was horrible due to the fact the GHA action step does much more we don't currently need.
So at this time, this seems the most vanilla and default way to override for a specific version of kustomize that what bundled in GHA automatically
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.
btw, by "vanilla and default way" I mean what indicated by the manual, here: https://kubectl.docs.kubernetes.io/installation/kustomize/binaries/
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rareddy, tarilabs 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 |
This commit adds rest api testing to the openshift-ci deployment
Resolves #42
Description
As part of KF 1.9 release checklist, make sure Kustomize 5.2.1. is used when testing the Model Registry manifests
Merge criteria: