- Notifications
You must be signed in to change notification settings - Fork927
chore(examples): update devcontainer-docker template#14199
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should this also be passed toenbuilder_cached_image? What about utilizing the outputenvbuilder_cached_image.cached.env?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oop, forgot about the env 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK, I took a stab at this -- unfortunately there's abug in the provider that mangles the init script. I can remove the FIXME after this is done though.
Uh oh!
There was an error while loading.Please reload this page.
1416edd tob8c3ddaCompareb8c3dda to3f37703Compare| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this actually sensitive if it's just the path to a file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🤷 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, but I'm going to keep it like this untilcoder/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Alternatively, we could validate at runtime thatextra_env does not contain any duplicated variables from the inputs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
| item { | ||
| key = "cache repo" | ||
| value = var.cache_repo == "" ? "not enabled" : var.cache_repo | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice addition ❤️
e978d4d intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Updates the devcontainer-docker template to use the coder/envbuilder provider.
Caching is completely optional here.