- Notifications
You must be signed in to change notification settings - Fork917
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 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
This PR modifies the gcp-devcontainer example template to includesupport for devcontainer caching using the envbuilder provider.
Uh oh!
There was an error while loading.Please reload this page.
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 useenvbuilder: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 whethercache_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 withoutenvbuilder? if so, should we add a new example likegcp-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.
Theenvbuilder provider only gets used ifcache_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 🎉
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| 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
Uh oh!
There was an error while loading.Please reload this page.
| 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 readinghttps://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>
53e5746 intomainUh oh!
There was an error while loading.Please reload this page.
This PR modifies the gcp-devcontainer example template to include support for devcontainer caching using the envbuilder provider.