- Notifications
You must be signed in to change notification settings - Fork4
feat: add coderd_template resource#35
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
ethanndickson commentedJul 18, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This stack of pull requests is managed by Graphite.Learn more about stacking. Join@ethanndickson and the rest of your teammates on |
40c3e02 toc0065c3Compare9d925c0 to534d0c9Comparec0065c3 to3e749a6Compare534d0c9 to3237486Compare3e749a6 to39670c5Compare3237486 to914db7eCompare39670c5 to2fca026Compare914db7e toeb0c94aCompare2fca026 to8222503Compareeb0c94a to5f8c0cfCompare8222503 to2632693Compare5f8c0cf toa7601b2Compare2632693 tocae8c17Comparea7601b2 to899e04eCompare92ecb54 tob35dec9Compare2fa4521 to64ed234Comparesreya commentedJul 22, 2024
I like nesting a template version within a template 👍 |
587b480 to509c7c3Compare509c7c3 toa30132aCompare| // varFiles, err := codersdk.DiscoverVarsFiles(directory) | ||
| // if err != nil { | ||
| // return nil, fmt.Errorf("failed to discover vars files: %s", err) | ||
| // } |
ethanndicksonJul 24, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
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.
I'll getcoder/coder#13984 cherry-picked into the next release, and then we can add this.
We can't use a go mod replace because there's breakingcodersdk changes in main where the respectivecoderd changes aren't in the docker image we use for tests.
Uh oh!
There was an error while loading.Please reload this page.
| Versions: []testAccTemplateVersionConfig{ | ||
| { | ||
| Name: PtrTo("main"), | ||
| Directory: PtrTo("../../integration/template-test/example-template/"), |
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.
Open to suggestions on better ways to do these acceptance tests.
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'm not sure it's possible to pass in a mockcodersdk.Client, but an alternative approach I've seen used in the wild would involve usinghttpmock in conjunction with responses as fixtures (example).
This has the benefit of not crossing the 'streams' between the acceptance and integration tests. But I think the current approach is OK for now.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
a30132a to37c1724CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
37c1724 tobd5fbd1Compare
Uh oh!
There was an error while loading.Please reload this page.
Design Rationale:
The RFC previously proposed defining templates and template versions separately. Multiple template version resources for a template could exist, only one of them needed to be marked as the active version (by ID) on the template resource.This could introduce a circular dependency between the two, as template versions require the ID of a template at creation (it cannot be added later), and templates must know the IDs of all template versions associated with it. To fix this, only the template version would point at the template, and not the other way around.
For example:
The Terraform internal logic would then be:
coderdto include the template version ID. If the template version is marked as active, set it as the active version.This updating of templates from template version resources feels a bit janky, or at least not Terraform idiomatic.
I therefore propose that the schema for creating templates using Terraform should instead look like:
This means we'd just do the third step in one resource creation task.
In a Netgru meeting, we decided we'd prioritise uploading template versions by having users supply a path to their directory. It's then pretty trivial to just have Terraform compute a hash of all the contents during planning, and use that to determine whether or not the template version needs updating.
However, we can't update the contents of a template version, we can only create a new version.
This means internally, a terraform representation of a template resource would only represent the state of the latest version, and the previously created versions would be effectively 'orphaned', with no way to modify their state on
coderdvia Terraform, outside of simply deleting thetemplateresource. I don't believe we can avoid this orphaning of versions, though I'm open to suggestions.This feels like the only surprising part of the schema; that each version in the versions list doesn't actually represent a single version on
coderd. In reality, these versions listed in the resource are more likeparallel branches of the template, and what's written in Terraform is merely the head of each branch, and where one of those branches can be marked as 'active'.(We can also just not support multiple template versions through Terraform as in the above snippet, and instead just have a template resource be given a single directory, the implementation is basically the same either way.)
I think this is our best option, but I'm keen to hear thoughts. With this, template creation is still idempotent, and we don't have any circular dependency issues.
Closes#29 and#30.
Todo: