Skip to content
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

Helm 3 preview #1543

Merged
merged 5 commits into from
Feb 16, 2020
Merged

Helm 3 preview #1543

merged 5 commits into from
Feb 16, 2020

Conversation

manics
Copy link
Member

@manics manics commented Jan 12, 2020

Adds Helm 3 to the Travis matrix.

Adds a Helm 3 preview doc.

Closes #1532

dev Show resolved Hide resolved
dev Outdated
@@ -56,7 +57,8 @@ def depend_on(binaries=[], envs=[]):
return decorator_depend_on


@depend_on(binaries=["kubectl", "kind", "docker"], envs=["Z2JH_KUBE_VERSION"])
@depend_on(binaries=["kubectl", "kind", "docker"],
envs=["Z2JH_KUBE_VERSION", "Z2JH_HELM_VERSION"])
Copy link
Member

Choose a reason for hiding this comment

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

We should probably not specify this here as we don't need it to have a value in the script. It made sense for me to introduce in order to enforce specifications of various variables.

Hmm... Thinking about it, if Z2JH_HELM_VERSION is omitted, we will use the currently available helm CLI, which could be v2 or v3. It would make more sense that the logic below checked for the helm version using the helm cli to determine how to act instead of probing this variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm a bit worried about autodetecting config in CI, because if someone makes an incorrect change to any of the CI scripts or Travis config the auto-configuration could skip over it and the tests would appear to pass, when in fact a portion of them might not have been run.

It could be made opt-in (add another flag e.g. dev --autodetect-env?) though that's still not very user friendly.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, we don't need to autodetect, but I think we should not require the explicit specification of the helm version using the depend_on decorator that will make the function fail unless it is specified. We are looking at the env var with a default of a blank string later anyhow.

So, not using the helm version to get the version of helm, but also not requiring the Z2JH_HELM_VERSION to be specified, makes sense to me short term. I want to revise this dev script again as soon as I find time btw. It is not good enough for local development :(

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand now, done!

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Excellent work on this @manics! ❤️ 🎉!

@manics
Copy link
Member Author

manics commented Jan 14, 2020

IIRC Circle CI will build and publish the PR docs, can you remind me how to find them? Can't see anything obvious in the job status https://circleci.com/gh/jupyterhub/zero-to-jupyterhub-k8s/817

@manics
Copy link
Member Author

manics commented Feb 14, 2020

@consideRatio Any more thoughts?

@consideRatio consideRatio merged commit 8068e1e into jupyterhub:master Feb 16, 2020
@consideRatio
Copy link
Member

❤️ thanks for your work on this @manics! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mention in docs that the helm section refers to helm2
2 participants