fix: Reduce variables needed for Docker template #3442
Conversation
examples/templates/docker/main.tf
Outdated
| } | ||
|
|
||
| provider "coder" { | ||
| # host = data.coder_provisioner.me.os == "windows" ? "npipe:////.//pipe//docker_engine" : "unix:///var/run/docker.sock" |
There was a problem hiding this comment.
why is this commented out? this works on MacOS but not sure if it would on Windows
There was a problem hiding this comment.
Oops, wasn't intentional. Fixed!
This should make initial setup a bit simpler!
fdb76f2 to
6aa6928
Compare
| @@ -84,7 +46,7 @@ variable "docker_image" { | |||
| } | |||
|
|
|||
| resource "docker_volume" "home_volume" { | |||
| name = "coder-${data.coder_workspace.me.owner}-${lower(data.coder_workspace.me.name)}-root" | |||
| name = "coder-${data.coder_workspace.me.owner_id}-${lower(data.coder_workspace.me.id)}-root" | |||
There was a problem hiding this comment.
This change might trigger #3386 for certain users who like to keep up-to-date with our templates. I think this is a change we really want, but I'd like for some warning/notifying facility to be in place before we do.
At minimum, we should list this as a breaking change in the release notes and warn of the behavior (update template + update workspace = volume loss).
There was a problem hiding this comment.
I'll add it to the release notes on the patch.
We advertise that users shouldn't stay up-to-date with our templates, so I'm not very concerned about updating.
There was a problem hiding this comment.
We advertise that users shouldn't stay up-to-date with our templates, so I'm not very concerned about updating.
Hmm, I don't see this mentioned in examples or e.g. the docker template?
Anyway, my main motivation behind being extra careful about #3386 & co. is that I don't want our current users to lose confidence in Coder because they lost their volume (even if it's due to their own configuration/changes).
There was a problem hiding this comment.
Fair enough. I can also exclude this if you feel it's sketchy. It's not needed for the improvement.
There was a problem hiding this comment.
I'm going to exclude it for right now. It's not worth the possible tumult for users.
There was a problem hiding this comment.
Fair enough. I can also exclude this if you feel it's sketchy. It's not needed for the improvement.
If that's ok, we could at least defer it until we make the change to all templates? Since I don't know if/when we'll have a solution for #3386, I won't ask us to defer until then. But doing it all at once minimizes the spread, at least.
PostgreSQL 13 doesn't support the M series architecture.
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
|
Hello @kylecarbs , could you please add reference to the issue this pull request is related to? |
This should make initial setup a bit simpler!