Skip to content

chore: allow pushing only inactive coderd_template versions #167

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

Merged
merged 2 commits into from
Jan 21, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/resources/template.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ resource "coderd_template" "ubuntu-main" {
- `description` (String) A description of the template.
- `display_name` (String) The display name of the template. Defaults to the template name.
- `failure_ttl_ms` (Number) (Enterprise) The max lifetime before Coder stops all resources for failed workspaces created from this template, in milliseconds.
- `icon` (String) Relative path or external URL that specifes an icon to be displayed in the dashboard.
- `icon` (String) Relative path or external URL that specifies an icon to be displayed in the dashboard.
- `max_port_share_level` (String) (Enterprise) The maximum port share level for workspaces created from this template. Defaults to `owner` on an Enterprise deployment, or `public` otherwise.
- `organization_id` (String) The ID of the organization. Defaults to the provider's default organization
- `require_active_version` (Boolean) (Enterprise) Whether workspaces must be created from the active version of this template. Defaults to false.
Expand Down
7 changes: 4 additions & 3 deletions integration/integration.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,10 @@ func StartCoder(ctx context.Context, t *testing.T, name string, useLicense bool)
ctr, err := cli.ContainerCreate(ctx, &container.Config{
Image: coderImg + ":" + coderVersion,
Env: []string{
"CODER_HTTP_ADDRESS=0.0.0.0:3000", // Listen on all interfaces inside the container
"CODER_ACCESS_URL=http://localhost:3000", // Set explicitly to avoid creating try.coder.app URLs.
"CODER_TELEMETRY_ENABLE=false", // Avoid creating noise.
"CODER_HTTP_ADDRESS=0.0.0.0:3000", // Listen on all interfaces inside the container
"CODER_ACCESS_URL=http://localhost:3000", // Set explicitly to avoid creating try.coder.app URLs.
"CODER_TELEMETRY_ENABLE=false", // Avoid creating noise.
"CODER_DANGEROUS_DISABLE_RATE_LIMITS=true", // Avoid hitting file rate limit in tests.
},
Labels: map[string]string{},
ExposedPorts: map[nat.Port]struct{}{nat.Port("3000/tcp"): {}},
Expand Down
2 changes: 1 addition & 1 deletion integration/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ func TestIntegration(t *testing.T) {
tfCmd.Stderr = &buf
tt.preF(t, client)
if err := tfCmd.Run(); !assert.NoError(t, err) {
t.Logf("%s", buf.String())
t.Log(buf.String())
t.FailNow()
}
tt.assertF(t, client)
Expand Down
109 changes: 90 additions & 19 deletions internal/provider/template_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ func (r *TemplateResource) Schema(ctx context.Context, req resource.SchemaReques
},
},
"icon": schema.StringAttribute{
MarkdownDescription: "Relative path or external URL that specifes an icon to be displayed in the dashboard.",
MarkdownDescription: "Relative path or external URL that specifies an icon to be displayed in the dashboard.",
Optional: true,
Computed: true,
Default: stringdefault.StaticString(""),
Expand Down Expand Up @@ -404,7 +404,7 @@ func (r *TemplateResource) Schema(ctx context.Context, req resource.SchemaReques
Required: true,
Validators: []validator.List{
listvalidator.SizeAtLeast(1),
NewActiveVersionValidator(),
NewVersionsValidator(),
},
NestedObject: schema.NestedAttributeObject{
Attributes: map[string]schema.Attribute{
Expand Down Expand Up @@ -867,24 +867,24 @@ func (r *TemplateResource) ConfigValidators(context.Context) []resource.ConfigVa
return []resource.ConfigValidator{}
}

type activeVersionValidator struct{}
type versionsValidator struct{}

func NewActiveVersionValidator() validator.List {
return &activeVersionValidator{}
func NewVersionsValidator() validator.List {
return &versionsValidator{}
}

// Description implements validator.List.
func (a *activeVersionValidator) Description(ctx context.Context) string {
func (a *versionsValidator) Description(ctx context.Context) string {
return a.MarkdownDescription(ctx)
}

// MarkdownDescription implements validator.List.
func (a *activeVersionValidator) MarkdownDescription(context.Context) string {
return "Validate that exactly one template version has active set to true."
func (a *versionsValidator) MarkdownDescription(context.Context) string {
return "Validate that template version names are unique and that at most one version is active."
}

// ValidateList implements validator.List.
func (a *activeVersionValidator) ValidateList(ctx context.Context, req validator.ListRequest, resp *validator.ListResponse) {
func (a *versionsValidator) ValidateList(ctx context.Context, req validator.ListRequest, resp *validator.ListResponse) {
if req.ConfigValue.IsNull() || req.ConfigValue.IsUnknown() {
return
}
Expand All @@ -908,13 +908,13 @@ func (a *activeVersionValidator) ValidateList(ctx context.Context, req validator
uniqueNames[version.Name.ValueString()] = struct{}{}
}

// Check if only one item in Version has active set to true
// Ensure at most one version is active
active := false
for _, version := range data {
// `active` is required, so if it's null or unknown, this is Terraform
// `active` defaults to false, so if it's null or unknown, this is Terraform
// requesting an early validation.
if version.Active.IsNull() || version.Active.IsUnknown() {
return
continue
}
if version.Active.ValueBool() {
if active {
Expand All @@ -924,12 +924,9 @@ func (a *activeVersionValidator) ValidateList(ctx context.Context, req validator
active = true
}
}
if !active {
resp.Diagnostics.AddError("Client Error", "At least one template version must be active.")
}
}

var _ validator.List = &activeVersionValidator{}
var _ validator.List = &versionsValidator{}

type versionsPlanModifier struct{}

Expand All @@ -956,6 +953,12 @@ func (d *versionsPlanModifier) PlanModifyList(ctx context.Context, req planmodif
return
}

hasActiveVersion, diag := hasOneActiveVersion(configVersions)
if diag.HasError() {
resp.Diagnostics.Append(diag...)
return
}

for i := range planVersions {
hash, err := computeDirectoryHash(planVersions[i].Directory.ValueString())
if err != nil {
Expand All @@ -974,6 +977,13 @@ func (d *versionsPlanModifier) PlanModifyList(ctx context.Context, req planmodif
// If this is the first read, init the private state value
if lvBytes == nil {
lv = make(LastVersionsByHash)
// If there's no prior private state, this might be resource creation,
// in which case one version must be active.
if !hasActiveVersion {
resp.Diagnostics.AddError("Client Error", "At least one template version must be active when creating a"+
" `coderd_template` resource.\n(Subsequent resource updates can be made without an active template in the list).")
return
}
} else {
err := json.Unmarshal(lvBytes, &lv)
if err != nil {
Expand All @@ -982,9 +992,34 @@ func (d *versionsPlanModifier) PlanModifyList(ctx context.Context, req planmodif
}
}

planVersions.reconcileVersionIDs(lv, configVersions)
diag = planVersions.reconcileVersionIDs(lv, configVersions, hasActiveVersion)
if diag.HasError() {
resp.Diagnostics.Append(diag...)
return
}

resp.PlanValue, resp.Diagnostics = types.ListValueFrom(ctx, req.PlanValue.ElementType(ctx), planVersions)
resp.PlanValue, diag = types.ListValueFrom(ctx, req.PlanValue.ElementType(ctx), planVersions)
if diag.HasError() {
resp.Diagnostics.Append(diag...)
}
}

func hasOneActiveVersion(data Versions) (hasActiveVersion bool, diags diag.Diagnostics) {
active := false
for _, version := range data {
if version.Active.IsNull() || version.Active.IsUnknown() {
// If null or unknown, the value will be defaulted to false
continue
}
if version.Active.ValueBool() {
if active {
diags.AddError("Client Error", "Only one template version can be active at a time.")
return
}
active = true
}
}
return active, diags
}

func NewVersionsPlanModifier() planmodifier.List {
Expand Down Expand Up @@ -1309,6 +1344,7 @@ type PreviousTemplateVersion struct {
ID uuid.UUID `json:"id"`
Name string `json:"name"`
TFVars map[string]string `json:"tf_vars"`
Active bool `json:"active"`
}

type privateState interface {
Expand All @@ -1331,13 +1367,15 @@ func (v Versions) setPrivateState(ctx context.Context, ps privateState) (diags d
ID: version.ID.ValueUUID(),
Name: version.Name.ValueString(),
TFVars: tfVars,
Active: version.Active.ValueBool(),
})
} else {
lv[version.DirectoryHash.ValueString()] = []PreviousTemplateVersion{
{
ID: version.ID.ValueUUID(),
Name: version.Name.ValueString(),
TFVars: tfVars,
Active: version.Active.ValueBool(),
},
}
}
Expand All @@ -1350,7 +1388,7 @@ func (v Versions) setPrivateState(ctx context.Context, ps privateState) (diags d
return ps.SetKey(ctx, LastVersionsKey, lvBytes)
}

func (planVersions Versions) reconcileVersionIDs(lv LastVersionsByHash, configVersions Versions) {
func (planVersions Versions) reconcileVersionIDs(lv LastVersionsByHash, configVersions Versions, hasOneActiveVersion bool) (diag diag.Diagnostics) {
// We remove versions that we've matched from `lv`, so make a copy for
// resolving tfvar changes at the end.
fullLv := make(LastVersionsByHash)
Expand Down Expand Up @@ -1420,6 +1458,39 @@ func (planVersions Versions) reconcileVersionIDs(lv LastVersionsByHash, configVe
}
}
}

// If a version was deactivated, and no active version was set, we need to
// return an error to avoid a post-apply plan being non-empty.
if !hasOneActiveVersion {
for i := range planVersions {
if !planVersions[i].ID.IsUnknown() {
prevs, ok := fullLv[planVersions[i].DirectoryHash.ValueString()]
if !ok {
continue
}
if versionDeactivated(prevs, &planVersions[i]) {
diag.AddError("Client Error", "Plan could not determine which version should be active.\n"+
"Either specify an active version or modify the contents of the previously active version before marking it as inactive.")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of returning an error and failing the plan here, we could just push a new, identical, template version and not promote it to active. Given there's no other way to push an unmodified template version, and the whole point of this resource is to avoid pushing spurious diffs, I think it's better to just fail the plan.

return diag
}
}
}
}
return diag
}

func versionDeactivated(prevs []PreviousTemplateVersion, planned *TemplateVersion) bool {
for _, prev := range prevs {
if prev.ID == planned.ID.ValueUUID() {
if prev.Active &&
!planned.Active.IsNull() &&
!planned.Active.IsUnknown() &&
!planned.Active.ValueBool() {
return true
}
}
}
return false
}

func tfVariablesChanged(prevs []PreviousTemplateVersion, planned *TemplateVersion) bool {
Expand Down
Loading
Loading