fix: Use immutable names for volumes in example templates #3663
Conversation
There was a problem hiding this comment.
Have these been tested? I expected some of these names have length limits which will be hit by stringing together two UUIDs. Why not only use the workspace ID instead of both the user ID and the workspace ID instead?
Also, I don't think these should be merged until we've decided what upgrade strategy we want to put into our changelog. Are we going to tell people that they need to make a new template and abandon their old workspaces?
|
Dogfood thing fails due to tagging with branch name (containing @deansheather I've tested Docker, but have not tested others (I can do a test on DO, but don't have setups for any others). Hmm, we could probably stick to only using the workspace UUID though 👍🏻. One motivation behind using both is additional metadata, but that would be amended by e.g. #3048. |
cc34cc6 to
ed59636
Compare
I've made the change to single UUID. 👍🏻
They can keep using their existing templates if they wish, but they should be vary of updating volume names to match that of these updated examples because that will result in loss of volumes.
Thoughts @deansheather? |
|
This Pull Request is becoming stale. In order to minimize WIP, prevent merge conflicts and keep the tracker readable, I'm going close to this PR in 3 days if there isn't more activity. |
|
This Pull Request is becoming stale. In order to minimize WIP, prevent merge conflicts and keep the tracker readable, I'm going close to this PR in 3 days if there isn't more activity. |
|
This Pull Request is becoming stale. In order to minimize WIP, prevent merge conflicts and keep the tracker readable, I'm going close to this PR in 3 days if there isn't more activity. |
|
@deansheather ping on this #3663 (comment) @deansheather @kylecarbs thoughts? Do we still want to merge this change? Or do we want to combine it with some other changes? Or do something else entirely? |
|
I'd change the text to say that we don't recommend updating templates, and instead renaming the old one and creating a new template so users can migrate their data across. Hopefully we don't need to do this again. |
|
Updated text:
|
| <<EOT | ||
| trap '[ $? -ne 0 ] && echo === Agent script exited with non-zero code. Sleeping infinitely to preserve logs... && sleep infinity' EXIT | ||
| ${replace(coder_agent.main.init_script, "/localhost|127\\.0\\.0\\.1/", "host.docker.internal")} | ||
| EOT | ||
| "sh", "-c", replace(coder_agent.main.init_script, "/localhost|127\\.0\\.0\\.1/", "host.docker.internal") |
There was a problem hiding this comment.
This may make debugging harder since the container will get immediately deleted AFAIK
There was a problem hiding this comment.
We already have a trap in the bootstrap script, I think this is a remnant from a past implementation before that change.
This one also uses sleep infinity which isn’t compatible with all docker images.
|
This Pull Request is becoming stale. In order to minimize WIP, prevent merge conflicts and keep the tracker readable, I'm going close to this PR in 3 days if there isn't more activity. |
|
We can use the |
It could definitely be a good addition, I'll try it out (I'm a bit worried about how Terraform will interpret this for existing resources that lack it, which might not help with the non-breakage). |
I tried it out already in our dogfood image and it doesn't invalidate the resource. The |
|
This Pull Request is becoming stale. In order to minimize WIP, prevent merge conflicts and keep the tracker readable, I'm going close to this PR in 3 days if there isn't more activity. |
This contributes towards #3000, #3386
Related #3409
NOTE: When this is merged, our release notes should make a mention / warning about updating to our example templates as doing so will very likely result in data loss (due to volume name changes).