-
Notifications
You must be signed in to change notification settings - Fork 887
feat(examples/templates/gcp-devcontainer): add envbuilder provider #14405
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
Conversation
This PR modifies the gcp-devcontainer example template to include support for devcontainer caching using the envbuilder provider.
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.
A minor suggestion to use the latest module version. Otherwise looks good to me.
Thank you for doing it 😊
} | ||
|
||
variable "cache_repo_docker_config_path" { | ||
default = "" |
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 there any standard path we can suggest here or mention in the description?
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'll mention ~/.docker/config.json
but it'll depend heavily on their setup.
data "coder_parameter" "devcontainer_builder" { | ||
description = <<-EOF | ||
Image that will build the devcontainer. | ||
We highly recommend using a specific release as the `:latest` tag will change. |
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 rephrase it to indicate it is dangerous? For example: Do not use envbuilder:latest
tag due to the risk of build instability.
# If we have a cached image, use the cached image's environment variables. Otherwise, just use | ||
# the environment variables we've defined above. | ||
docker_env_input = try(envbuilder_cached_image.cached.0.env_map, local.envbuilder_env) | ||
# Convert the above to the list of arguments for the Docker run command. This is going to end | ||
# up in our startup script metadata. These are all terminated by backslashes. | ||
docker_env_arg_list = [for k, v in local.docker_env_input : " -e \"${k}=${v}\" \\"] |
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 user modify these locals? If not, maybe indicate that in a comment.
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.
yeah probably not :D
type = string | ||
} | ||
|
||
variable "insecure_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.
This changeset is relatively big, so is the .tf
file. Maybe wipe out this variable?
variable "project_id" { | ||
description = "Which Google Compute Project should your workspace live in?" | ||
} | ||
|
||
variable "cache_repo" { | ||
default = "" | ||
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.
People may be struggling whether cache_repo
is a path, URI, endpoint, etc.
@@ -6,16 +6,49 @@ terraform { | |||
google = { | |||
source = "hashicorp/google" | |||
} | |||
envbuilder = { |
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.
curious: will we support the original form without envbuilder
? if so, should we add a new example like gcp-devcontainer
?
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.
The envbuilder
provider only gets used if cache_repo
is set.
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 work 🎉
docker_env_input = try(envbuilder_cached_image.cached.0.env_map, local.envbuilder_env) | ||
# Convert the above to the list of arguments for the Docker run command. This is going to end | ||
# up in our startup script metadata. These are all terminated by backslashes. | ||
docker_env_arg_list = [for k, v in local.docker_env_input : " -e \"${k}=${v}\" \\"] |
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.
This conversion might have trouble with some inputs, writing to an env file and referencing that is an alternative as it doesn't require quoting. Newline inputs could still spell trouble though.
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.
Something like this?
printf %s "<base64-encoded string>" | base64 -d > env.list
auth = "token" | ||
os = "linux" | ||
dir = "/workspaces/${trimsuffix(basename(data.coder_parameter.repo_url.value), ".git")}" | ||
connection_timeout = 0 |
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.
Does setting this to zero actually serve a purpose? Doesn't it just use the default of 30s in this case?
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.
Unsure; this was from the original template IIRC.
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 from reading https://registry.terraform.io/providers/coder/coder/latest/docs/resources/agent#connection_timeout I could imagine that this is set to avoid the agent showing up as 'timed out' due to the GCP instance potentially taking a long time to start.
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com> Co-authored-by: Muhammad Atif Ali <atif@coder.com>
This PR modifies the gcp-devcontainer example template to include support for devcontainer caching using the envbuilder provider.