unrevert "tiltfile: disallow deploys to remote kube by default" #2012
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
August 7, 2019 17:14
nicks
approved these changes
Aug 7, 2019
internal/tiltfile/tiltfile_state.go
Outdated
| // These are k8s context names that we assume are safe to deploy to even if they are neither localhost | ||
| // nor in allow_k8s_contexts. e.g., minikube uses a non-loopback ip on a virtual interface, | ||
| // and some versions of docker-for-desktop use 'kubernetes.docker.local' | ||
| var defaultWhitelistedKubeContexts = []k8s.KubeContext{"minikube", "docker-desktop", "docker-for-desktop"} |
There was a problem hiding this comment.
my main concern is having multiple codepaths that each invent their own definition of "local" cluster.
I'd forgotten about Env when writing this. Do you see any downside to just using Env.IsLocalCluster and skip checking the url?
I'm not very familiar with KIND, but it seems like the chief current use of Env.IsLocalCluster is to check whether can skip the push to docker, and we push to docker for KIND even though it's local? Which is to say, I'm not sure whether this method's goals align with the safety check's. Either way, though, it seems like it makes sense to put this logic in env.go.
There was a problem hiding this comment.
OK, new take:
- renamed the existing
Env.IsLocalClustertoEnv.UsesLocalDockerRegistry, since that seems to better match its existing use, and then made a newEnv.IsLocalClusterthat also includes KinD. - just use
Env.IsLocalClusterand don't get clever in the tiltfile code
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(re-)resolves #1096
#1995 was reverted because it turns out the latest Docker for Desktop doesn't use "localhost" as its kube api, but instead adds kubernetes.docker.local to /etc/hosts, and uses that.
This is a two-commit PR:
#1 Revert the revert (i.e., exactly what was already approved)
#2 Add Docker for Desktop context names to the whitelist