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

docker: change how we model "image builds show up in the cluster immediately" #4598

Merged
merged 2 commits into from
Jun 7, 2021

Conversation

nicks
Copy link
Member

@nicks nicks commented May 27, 2021

Hello @maiamcc,

Please review the following commits I made in branch nicks/ch12079:

cb46eee (2021-05-27 17:28:54 -0400)
docker: change how we model "image builds show up in the cluster immediately"
We used to treat this as a property of the cluster type + the container runtime.

But this made it impossible to support clusters that sometimes use your docker
runtime, and sometimes do not.

For examples, see:

This changes the data model so that "image builds show up in the cluster"
is a property of the Docker client, not of the cluster.

This should be much more flexible and correct, and help us support multiple
clusters.

Fixes #4544

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

…diately"

We used to treat this as a property of the cluster type + the container runtime.

But this made it impossible to support clusters that sometimes use your docker
runtime, and sometimes do not.

For examples, see:
- #4587
- #3654
- #1729
- #4544

This changes the data model so that "image builds show up in the cluster"
is a property of the Docker client, not of the cluster.

This should be much more flexible and correct, and help us support multiple
clusters.

Fixes #4544
@nicks nicks requested a review from maiamcc May 27, 2021 21:34
@shortcut-integration
Copy link

internal/docker/env.go Outdated Show resolved Hide resolved
Co-authored-by: Maia McCormick <maia@windmill.engineering>
@nicks nicks merged commit bc91f22 into master Jun 7, 2021
@nicks nicks deleted the nicks/ch12079 branch June 7, 2021 17:19
landism pushed a commit that referenced this pull request Jun 9, 2021
…diately" (#4598)

* docker: change how we model "image builds show up in the cluster immediately"

We used to treat this as a property of the cluster type + the container runtime.

But this made it impossible to support clusters that sometimes use your docker
runtime, and sometimes do not.

For examples, see:
- #4587
- #3654
- #1729
- #4544

This changes the data model so that "image builds show up in the cluster"
is a property of the Docker client, not of the cluster.

This should be much more flexible and correct, and help us support multiple
clusters.

Fixes #4544

* Update internal/docker/env.go

Co-authored-by: Maia McCormick <maia@windmill.engineering>

Co-authored-by: Maia McCormick <maia@windmill.engineering>
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.

Can't start tilt with multi node minikube clusters using docker driver
2 participants