chore(examples): update devcontainer-docker template #14199
Conversation
| @@ -161,7 +174,10 @@ resource "docker_container" "workspace" { | |||
| "ENVBUILDER_FALLBACK_IMAGE=${data.coder_parameter.fallback_image.value}", | |||
| "ENVBUILDER_CACHE_REPO=${var.cache_repo}", | |||
| "ENVBUILDER_DOCKER_CONFIG_BASE64=${try(data.local_sensitive_file.cache_repo_dockerconfigjson[0].content_base64, "")}", | |||
There was a problem hiding this comment.
Should this also be passed to enbuilder_cached_image? What about utilizing the output envbuilder_cached_image.cached.env?
There was a problem hiding this comment.
oop, forgot about the env 👍
There was a problem hiding this comment.
OK, I took a stab at this -- unfortunately there's a bug in the provider that mangles the init script. I can remove the FIXME after this is done though.
1416edd to
b8c3dda
Compare
b8c3dda to
3f37703
Compare
| @@ -89,14 +93,13 @@ EOF | |||
|
|
|||
| variable "cache_repo" { | |||
| default = "" | |||
| description = "Use a container registry as a cache to speed up builds." | |||
| sensitive = true | |||
| description = "(Optional) Use a container registry as a cache to speed up builds." | |||
There was a problem hiding this comment.
review: this shouldn't contain any credentials and it's kind of annoying to have this sensitive
| type = string | ||
| } | ||
|
|
||
| variable "cache_repo_docker_config_path" { | ||
| default = "" | ||
| description = "Path to a docker config.json containing credentials to the provided cache repo, if required." | ||
| description = "(Optional) Path to a docker config.json containing credentials to the provided cache repo, if required." | ||
| sensitive = true |
There was a problem hiding this comment.
Is this actually sensitive if it's just the path to a file?
There was a problem hiding this comment.
🤷 maybe? The nice thing about making the file sensitive is that the content automatically becomes sensitive. (That's my recollection anyhow)
There was a problem hiding this comment.
Ok, so there’s some magic there wrt content? If so please keep it as is 👍
| "ENVBUILDER_DOCKER_CONFIG_BASE64" : try(data.local_sensitive_file.cache_repo_dockerconfigjson[0].content_base64, ""), | ||
| "ENVBUILDER_PUSH_IMAGE" : var.cache_repo == "" ? "" : "true", | ||
| #"ENVBUILDER_INSECURE": "true", # Uncomment if testing with an insecure registry. | ||
| } |
There was a problem hiding this comment.
If these are fed in via extra env, shouldn't they be included in the computed env the resource outputs? (In which case the below would be unnecessary?)
There was a problem hiding this comment.
Yes, but I'm going to keep it like this until coder/terraform-provider-envbuilder#31 is fixed.
| count = var.cache_repo == "" ? 0 : data.coder_workspace.me.start_count | ||
| builder_image = local.devcontainer_builder_image | ||
| git_url = local.repo_url | ||
| cache_repo = var.cache_repo |
There was a problem hiding this comment.
Should we make these optional and let them be provided via extra env too? Would simplify a bit and a user knows they can just cram everything in extra.
Right now for url is given here and in extra, what if the values differ? What's the behavior? (IMO maybe it's an error).
There was a problem hiding this comment.
Alternatively, we could validate at runtime that extra_env does not contain any duplicated variables from the inputs?
There was a problem hiding this comment.
I agree, that’s what I had in mind, but in a more roundabout way 😅. I think it would help with avoiding mistakes, and remove ambiguity.
| item { | ||
| key = "cache repo" | ||
| value = var.cache_repo == "" ? "not enabled" : var.cache_repo | ||
| } |
Updates the devcontainer-docker template to use the coder/envbuilder provider.
Caching is completely optional here.