-
Notifications
You must be signed in to change notification settings - Fork 35
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
Offer to install helm into venv #610
Offer to install helm into venv #610
Conversation
src/warnet/constants.py
Outdated
@@ -98,3 +98,8 @@ | |||
f"helm upgrade --install grafana-dashboards {CHARTS_DIR}/grafana-dashboards --namespace warnet-logging", | |||
f"helm upgrade --install --namespace warnet-logging loki-grafana grafana/grafana --values {MANIFESTS_DIR}/grafana_values.yaml", | |||
] | |||
|
|||
# Helm binary | |||
HELM_LATEST_URL = "https://get.helm.sh/helm-latest-version" |
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 think we should pin a version and also commit to the expected checksums:
https://github.com/helm/helm/releases
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 decided to bless v3.16.1 and always pull it: 07104ee
This is fucking dope dude, great work |
Yeah this is neat! Perhaps it would help us to define the install methods we will offer/support (and stick to it!). Something like:
Each of the methods should cover all required binaries. So for example I'd propose to follow this PR here with a similar If a user can't/won't install using one of these three ways, then they can't use the tool. This would hopefully stop this fiasco re-ocurring in the future (or at least, we wouldn't have to worry about it). |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
41335d4
to
1d49f06
Compare
Get the blessed version instead of the latest version.
@mplsgrant do you have a preference between which of these goes first? It looks like this one is perhaps more ready? |
Install looks good but i got this error trying to deploy a network with logging. Not sure if this is an issue with the pinned version of Helm or something wrong with the loki chart in the repo? warnet error:
So i tried to install using helm directly instead of through warnet...
|
@pinheadmz I noticed an issue with the architecture mapping. I'm not sure if that had something to do with the error you encountered, but I fixed that and it might be worth trying again. |
@willcl-ark I don't have a preference about which goes first. I'm fine to rebase either one. |
ACK ec33ace |
We no longer need the logic for the latest version of helm. Also we don't need the NeedsHelm option in ToolStatus.
Just wanted to remove a bit of dead code before merging. |
On Arm Mac I got: Also the question on setup is truncated: [?] Would you like to use Warnet's downloader to install Helm into your virtual environme...: |
The truncation must be because I run narrow terminals. Perhaps the question could be a bit shorter? Assuming |
@m3dwards What happens when you run this in a python terminal? import os
os.uname().machine |
|
Show user their arch instead of showing them "None"
Works! Great PR! |
post merge ACK, |
This offers to install helm into the user's venv directory. Fixes #608
I overloaded the
is_helm_installed
function a bit, but I figured that even if we had to do this for every dependency it would remain manageable.