Skip to content

Commit 1d19415

Browse files
ethanndicksonethanndickson
authored
fix: avoid sending template metadata update if max_port_share_level is default (#190)
Closes #188. For context, the `MaxPortShareLevel` template metadata value was not present on the `coderd` create template request prior to 2.15. As such, during template creation we need to send an update request for backwards compatibility (though we could probably remove this soon), see #110. Whilst we don't send this update request is the attribute is omitted on the resource, we were sending a spurious update request if the value was explicitly configured to the default value ('owner' for enterprise+, 'public' otherwise). This was causing the error in the linked issue. An example is seen in the newly added test. The fix is to just not send that update request if the configured value is: - Unknown - Equal to the value on the newly created template. note: I'd recommend hiding the whitespace when reviewing the diff
1 parent aa3dbcd commit 1d19415

File tree

2 files changed

+144
-111
lines changed

2 files changed

+144
-111
lines changed

‎internal/provider/template_resource.go

+2
Original file line numberDiff line numberDiff line change
@@ -573,6 +573,8 @@ func (r *TemplateResource) Create(ctx context.Context, req resource.CreateReques
573573
// deployment running `v2.15.0` or later.
574574
if data.MaxPortShareLevel.IsUnknown() {
575575
data.MaxPortShareLevel = types.StringValue(string(templateResp.MaxPortShareLevel))
576+
} else if data.MaxPortShareLevel.ValueString() == string(templateResp.MaxPortShareLevel) {
577+
tflog.Info(ctx, "max port share level set to default, not updating")
576578
} else {
577579
mpslReq := data.toUpdateRequest(ctx, &resp.Diagnostics)
578580
if resp.Diagnostics.HasError() {

‎internal/provider/template_resource_test.go

+142-111
Original file line numberDiff line numberDiff line change
@@ -657,129 +657,160 @@ func TestAccTemplateResourceEnterprise(t *testing.T) {
657657
err = cp.Copy("../../integration/template-test/example-template", exTemplateOne)
658658
require.NoError(t, err)
659659

660-
cfg1 := testAccTemplateResourceConfig{
661-
URL: client.URL.String(),
662-
Token: client.SessionToken(),
663-
Name: ptr.Ref("example-template"),
664-
Versions: []testAccTemplateVersionConfig{
665-
{
666-
// Auto-generated version name
667-
Directory: &exTemplateOne,
668-
Active: ptr.Ref(true),
669-
},
670-
},
671-
ACL: testAccTemplateACLConfig{
672-
GroupACL: []testAccTemplateKeyValueConfig{
673-
{
674-
Key: ptr.Ref(firstUser.OrganizationIDs[0].String()),
675-
Value: ptr.Ref("use"),
676-
},
660+
t.Run("BasicUsage", func(t *testing.T) {
661+
cfg1 := testAccTemplateResourceConfig{
662+
URL: client.URL.String(),
663+
Token: client.SessionToken(),
664+
Name: ptr.Ref("example-template"),
665+
Versions: []testAccTemplateVersionConfig{
677666
{
678-
Key: ptr.Ref(group.ID.String()),
679-
Value: ptr.Ref("admin"),
667+
// Auto-generated version name
668+
Directory: &exTemplateOne,
669+
Active: ptr.Ref(true),
680670
},
681671
},
682-
UserACL: []testAccTemplateKeyValueConfig{
683-
{
684-
Key: ptr.Ref(firstUser.ID.String()),
685-
Value: ptr.Ref("admin"),
672+
ACL: testAccTemplateACLConfig{
673+
GroupACL: []testAccTemplateKeyValueConfig{
674+
{
675+
Key: ptr.Ref(firstUser.OrganizationIDs[0].String()),
676+
Value: ptr.Ref("use"),
677+
},
678+
{
679+
Key: ptr.Ref(group.ID.String()),
680+
Value: ptr.Ref("admin"),
681+
},
682+
},
683+
UserACL: []testAccTemplateKeyValueConfig{
684+
{
685+
Key: ptr.Ref(firstUser.ID.String()),
686+
Value: ptr.Ref("admin"),
687+
},
686688
},
687689
},
688-
},
689-
}
690+
}
690691

691-
cfg2 := cfg1
692-
cfg2.ACL.GroupACL = slices.Clone(cfg2.ACL.GroupACL[1:])
693-
cfg2.MaxPortShareLevel = ptr.Ref("owner")
692+
cfg2 := cfg1
693+
cfg2.ACL.GroupACL = slices.Clone(cfg2.ACL.GroupACL[1:])
694+
cfg2.MaxPortShareLevel = ptr.Ref("owner")
694695

695-
cfg3 := cfg2
696-
cfg3.ACL.null = true
697-
cfg3.MaxPortShareLevel = ptr.Ref("public")
696+
cfg3 := cfg2
697+
cfg3.ACL.null = true
698+
cfg3.MaxPortShareLevel = ptr.Ref("public")
698699

699-
cfg4 := cfg3
700-
cfg4.AllowUserAutostart = ptr.Ref(false)
701-
cfg4.AutostopRequirement = testAccAutostopRequirementConfig{
702-
DaysOfWeek: ptr.Ref([]string{"monday", "tuesday"}),
703-
Weeks: ptr.Ref(int64(2)),
704-
}
700+
cfg4 := cfg3
701+
cfg4.AllowUserAutostart = ptr.Ref(false)
702+
cfg4.AutostopRequirement = testAccAutostopRequirementConfig{
703+
DaysOfWeek: ptr.Ref([]string{"monday", "tuesday"}),
704+
Weeks: ptr.Ref(int64(2)),
705+
}
705706

706-
resource.Test(t, resource.TestCase{
707-
PreCheck: func() { testAccPreCheck(t) },
708-
IsUnitTest: true,
709-
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
710-
Steps: []resource.TestStep{
711-
{
712-
Config: cfg1.String(t),
713-
Check: resource.ComposeAggregateTestCheckFunc(
714-
resource.TestCheckResourceAttr("coderd_template.test", "max_port_share_level", "owner"),
715-
resource.TestCheckResourceAttr("coderd_template.test", "acl.groups.#", "2"),
716-
resource.TestMatchTypeSetElemNestedAttrs("coderd_template.test", "acl.groups.*", map[string]*regexp.Regexp{
717-
"id": regexp.MustCompile(firstUser.OrganizationIDs[0].String()),
718-
"role": regexp.MustCompile("^use$"),
719-
}),
720-
resource.TestMatchTypeSetElemNestedAttrs("coderd_template.test", "acl.groups.*", map[string]*regexp.Regexp{
721-
"id": regexp.MustCompile(group.ID.String()),
722-
"role": regexp.MustCompile("^admin$"),
723-
}),
724-
resource.TestCheckResourceAttr("coderd_template.test", "acl.users.#", "1"),
725-
resource.TestMatchTypeSetElemNestedAttrs("coderd_template.test", "acl.users.*", map[string]*regexp.Regexp{
726-
"id": regexp.MustCompile(firstUser.ID.String()),
727-
"role": regexp.MustCompile("^admin$"),
728-
}),
729-
),
730-
},
731-
{
732-
Config: cfg2.String(t),
733-
Check: resource.ComposeAggregateTestCheckFunc(
734-
resource.TestCheckResourceAttr("coderd_template.test", "max_port_share_level", "owner"),
735-
resource.TestMatchTypeSetElemNestedAttrs("coderd_template.test", "acl.users.*", map[string]*regexp.Regexp{
736-
"id": regexp.MustCompile(firstUser.ID.String()),
737-
"role": regexp.MustCompile("^admin$"),
738-
}),
739-
),
707+
resource.Test(t, resource.TestCase{
708+
PreCheck: func() { testAccPreCheck(t) },
709+
IsUnitTest: true,
710+
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
711+
Steps: []resource.TestStep{
712+
{
713+
Config: cfg1.String(t),
714+
Check: resource.ComposeAggregateTestCheckFunc(
715+
resource.TestCheckResourceAttr("coderd_template.test", "max_port_share_level", "owner"),
716+
resource.TestCheckResourceAttr("coderd_template.test", "acl.groups.#", "2"),
717+
resource.TestMatchTypeSetElemNestedAttrs("coderd_template.test", "acl.groups.*", map[string]*regexp.Regexp{
718+
"id": regexp.MustCompile(firstUser.OrganizationIDs[0].String()),
719+
"role": regexp.MustCompile("^use$"),
720+
}),
721+
resource.TestMatchTypeSetElemNestedAttrs("coderd_template.test", "acl.groups.*", map[string]*regexp.Regexp{
722+
"id": regexp.MustCompile(group.ID.String()),
723+
"role": regexp.MustCompile("^admin$"),
724+
}),
725+
resource.TestCheckResourceAttr("coderd_template.test", "acl.users.#", "1"),
726+
resource.TestMatchTypeSetElemNestedAttrs("coderd_template.test", "acl.users.*", map[string]*regexp.Regexp{
727+
"id": regexp.MustCompile(firstUser.ID.String()),
728+
"role": regexp.MustCompile("^admin$"),
729+
}),
730+
),
731+
},
732+
{
733+
Config: cfg2.String(t),
734+
Check: resource.ComposeAggregateTestCheckFunc(
735+
resource.TestCheckResourceAttr("coderd_template.test", "max_port_share_level", "owner"),
736+
resource.TestMatchTypeSetElemNestedAttrs("coderd_template.test", "acl.users.*", map[string]*regexp.Regexp{
737+
"id": regexp.MustCompile(firstUser.ID.String()),
738+
"role": regexp.MustCompile("^admin$"),
739+
}),
740+
),
741+
},
742+
{
743+
Config: cfg3.String(t),
744+
Check: resource.ComposeAggregateTestCheckFunc(
745+
resource.TestCheckResourceAttr("coderd_template.test", "max_port_share_level", "public"),
746+
resource.TestCheckNoResourceAttr("coderd_template.test", "acl"),
747+
func(s *terraform.State) error {
748+
templates, err := client.Templates(ctx, codersdk.TemplateFilter{})
749+
if err != nil {
750+
return err
751+
}
752+
if len(templates) != 1 {
753+
return fmt.Errorf("expected 1 template, got %d", len(templates))
754+
}
755+
acl, err := client.TemplateACL(ctx, templates[0].ID)
756+
if err != nil {
757+
return err
758+
}
759+
if len(acl.Groups) != 1 {
760+
return fmt.Errorf("expected 1 group ACL, got %d", len(acl.Groups))
761+
}
762+
if acl.Groups[0].Role != "admin" && acl.Groups[0].ID != group.ID {
763+
return fmt.Errorf("expected group ACL to be 'use' for %s, got %s", firstUser.OrganizationIDs[0].String(), acl.Groups[0].Role)
764+
}
765+
if len(acl.Users) != 1 {
766+
return fmt.Errorf("expected 1 user ACL, got %d", len(acl.Users))
767+
}
768+
if acl.Users[0].Role != "admin" && acl.Users[0].ID != firstUser.ID {
769+
return fmt.Errorf("expected user ACL to be 'admin' for %s, got %s", firstUser.ID.String(), acl.Users[0].Role)
770+
}
771+
return nil
772+
},
773+
),
774+
},
775+
{
776+
Config: cfg4.String(t),
777+
Check: resource.ComposeAggregateTestCheckFunc(
778+
resource.TestCheckResourceAttr("coderd_template.test", "allow_user_auto_start", "false"),
779+
resource.TestCheckResourceAttr("coderd_template.test", "auto_stop_requirement.days_of_week.#", "2"),
780+
resource.TestCheckResourceAttr("coderd_template.test", "auto_stop_requirement.weeks", "2"),
781+
),
782+
},
740783
},
741-
{
742-
Config: cfg3.String(t),
743-
Check: resource.ComposeAggregateTestCheckFunc(
744-
resource.TestCheckResourceAttr("coderd_template.test", "max_port_share_level", "public"),
745-
resource.TestCheckNoResourceAttr("coderd_template.test", "acl"),
746-
func(s *terraform.State) error {
747-
templates, err := client.Templates(ctx, codersdk.TemplateFilter{})
748-
if err != nil {
749-
return err
750-
}
751-
if len(templates) != 1 {
752-
return fmt.Errorf("expected 1 template, got %d", len(templates))
753-
}
754-
acl, err := client.TemplateACL(ctx, templates[0].ID)
755-
if err != nil {
756-
return err
757-
}
758-
if len(acl.Groups) != 1 {
759-
return fmt.Errorf("expected 1 group ACL, got %d", len(acl.Groups))
760-
}
761-
if acl.Groups[0].Role != "admin" && acl.Groups[0].ID != group.ID {
762-
return fmt.Errorf("expected group ACL to be 'use' for %s, got %s", firstUser.OrganizationIDs[0].String(), acl.Groups[0].Role)
763-
}
764-
if len(acl.Users) != 1 {
765-
return fmt.Errorf("expected 1 user ACL, got %d", len(acl.Users))
766-
}
767-
if acl.Users[0].Role != "admin" && acl.Users[0].ID != firstUser.ID {
768-
return fmt.Errorf("expected user ACL to be 'admin' for %s, got %s", firstUser.ID.String(), acl.Users[0].Role)
769-
}
770-
return nil
771-
},
772-
),
784+
})
785+
})
786+
787+
// Verifies that when `max_port_share_level` is set to to the default value,
788+
// an update request that would return HTTP Not Modified is not sent.
789+
t.Run("DefaultMaxPortShareLevel", func(t *testing.T) {
790+
cfg1 := testAccTemplateResourceConfig{
791+
URL: client.URL.String(),
792+
Token: client.SessionToken(),
793+
Name: ptr.Ref("example-template"),
794+
Versions: []testAccTemplateVersionConfig{
795+
{
796+
Directory: &exTemplateOne,
797+
Active: ptr.Ref(true),
798+
},
773799
},
774-
{
775-
Config: cfg4.String(t),
776-
Check: resource.ComposeAggregateTestCheckFunc(
777-
resource.TestCheckResourceAttr("coderd_template.test", "allow_user_auto_start", "false"),
778-
resource.TestCheckResourceAttr("coderd_template.test", "auto_stop_requirement.days_of_week.#", "2"),
779-
resource.TestCheckResourceAttr("coderd_template.test", "auto_stop_requirement.weeks", "2"),
780-
),
800+
MaxPortShareLevel: ptr.Ref("owner"),
801+
}
802+
803+
resource.Test(t, resource.TestCase{
804+
PreCheck: func() { testAccPreCheck(t) },
805+
IsUnitTest: true,
806+
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
807+
Steps: []resource.TestStep{
808+
{
809+
Config: cfg1.String(t),
810+
Check: resource.TestCheckResourceAttr("coderd_template.test", "max_port_share_level", "owner"),
811+
},
781812
},
782-
},
813+
})
783814
})
784815
}
785816

0 commit comments

Comments
 (0)