Fix problems including missing charts and the kubeconfig permission#7033
Fix problems including missing charts and the kubeconfig permission#7033popojk merged 3 commits intoflyteorg:v2from
Conversation
Signed-off-by: yuteng <a08h0283@gmail.com>
|
I can reproduce the first issue on macOS by running cd ../../charts/flyte-sandbox/charts && for f in *.tgz; do tar xzf "$f"; done
tar: Error opening archive: Failed to open '*.tgz'
make[1]: *** [dep_update] Error 1
make: *** [sandbox-build] Error 2 |
docker/sandbox-bundled/Makefile
Outdated
| .PHONY: dep_update | ||
| dep_update: | ||
| @if [ ! -d ../../charts/flyte-sandbox/charts ] || [ -z "$$(find ../../charts/flyte-sandbox/charts -maxdepth 1 -name '*.tgz' -print -quit)" ]; then \ | ||
| cd ../../charts/flyte-sandbox && helm dependency update; \ |
There was a problem hiding this comment.
Should we use helm dependency build instead? This will reconstruct the chart dependencies without updating the lock. I think this is better to not update Chart.lock every time we do make sandbox-build
https://helm.sh/docs/helm/helm_dependency_build
Maybe we can add new dep_build command to build, and have another make command to do the update, so that we can use if we want to update .lock file
There was a problem hiding this comment.
And to use helm dependency build, we should add a new make command to add helm repos
.PHONY: helm-repos
helm-repos:
helm repo add docker-registry https://twuni.github.io/docker-registry.helm || true
helm repo add kubernetes-dashboard https://kubernetes-retired.github.io/dashboard/ || true
helm repo add bitnami https://charts.bitnami.com/bitnami || true
helm repo update
docker/sandbox-bundled/Makefile
Outdated
| @if [ ! -d ../../charts/flyte-sandbox/charts ] || [ -z "$$(find ../../charts/flyte-sandbox/charts -maxdepth 1 -name '*.tgz' -print -quit)" ]; then \ | ||
| cd ../../charts/flyte-sandbox && helm dependency update; \ | ||
| fi |
There was a problem hiding this comment.
Also, I think it's better to rebuild on every dep_update, to prevent having stale dependencies.
cd ../../charts/flyte-sandbox && helm dependency build|
I didn't get the second issue on my side (mac OS). But I think we can keep this change to support running on wsl ❯ make sandbox-run
Makefile:30: warning: overriding commands for target `build'
go.Makefile:75: warning: ignoring old commands for target `build'
Makefile:53: warning: overriding commands for target `help'
go.Makefile:25: warning: ignoring old commands for target `help'
/Library/Developer/CommandLineTools/usr/bin/make -C docker/sandbox-bundled start
[ -n "flyte-sandbox" ] || \
docker volume create flyte-sandbox
6773d779b5f13d71366555447ded4d288d2b92185773c7df57706d463ade637c
Waiting for kubeconfig...
Kubeconfig copied to ~/.flyte/sandboxv2/kubeconfig |
| fi | ||
| @echo "Waiting for kubeconfig..." | ||
| @until [ -s $(PWD)/.kube/kubeconfig ]; do sleep 1; done | ||
| @docker exec flyte-sandbox chown $(shell id -u):$(shell id -g) /.kube/kubeconfig |
There was a problem hiding this comment.
Let's leave a comment here showing that this is for WSL
# On WSL, the bind-mounted kubeconfig may be root-owned, which makes the host-side cp fail with permission denied.Signed-off-by: yuteng <a08h0283@gmail.com>
|
@machichima Thanks for comments. I added mentioned suggestions into the next commit. Proposals
Screenshot
|
docker/sandbox-bundled/Makefile
Outdated
| helm repo add docker-registry https://twuni.github.io/docker-registry.helm || true | ||
| helm repo add kubernetes-dashboard https://kubernetes-retired.github.io/dashboard/ || true | ||
| helm repo add bitnami https://charts.bitnami.com/bitnami || true |
There was a problem hiding this comment.
Curious, why do we need to add || true here? If the repo already exists, we will not get error. With and without || true will give the same output
❯ helm repo add bitnami https://charts.bitnami.com/bitnami || true
"bitnami" already exists with the same configuration, skipping
❯ helm repo add bitnami https://charts.bitnami.com/bitnami
"bitnami" already exists with the same configuration, skipping
There was a problem hiding this comment.
I mistakenly copied the comments including helm sources and overlooked removing the || true comments.
I will remove them properly.
Signed-off-by: yuteng <a08h0283@gmail.com>
machichima
left a comment
There was a problem hiding this comment.
LGTM! I can start sandbox successfully and run examples
cc @popojk for final check
…7033) * fix problems about missing charts and kubeconfig permission Signed-off-by: yuteng <a08h0283@gmail.com> * make dep_build including updating helm-repo and build charts Signed-off-by: yuteng <a08h0283@gmail.com> * deleted unused operator Signed-off-by: yuteng <a08h0283@gmail.com> --------- Signed-off-by: yuteng <a08h0283@gmail.com>

Tracking issue
#Close #7032
Why are the changes needed?
make sandbox-buildfailure caused by missing directory.make sandbox-runcommand to copy it.What changes were proposed in this pull request?
helm dependency updatecreates charts and downloads related tar if missing directory or empty directory happensHow was this patch tested?
<this pr branch>Labels
Setup process
Screenshots
Related PRs
Docs link