Skip to content
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.

Sign up for GitHub

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

use TypeList schema for gzip block attributes #433

Closed
wants to merge 4 commits into from
Closed

use TypeList schema for gzip block attributes #433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bcebf65
use TypeList schema for gzip block attributes
smaeda-ks Jun 26, 2021
9cbfb24
doc update
smaeda-ks Jun 26, 2021
4f69562
apply feedback
smaeda-ks Jun 28, 2021
3744637
Update fastly/block_fastly_service_v1_gzip.go
smaeda-ks Jun 28, 2021
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
4 changes: 2 additions & 2 deletions docs/resources/service_v1.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,8 +510,8 @@ Required:
Optional:

- **cache_condition** (String) Name of already defined `condition` controlling when this gzip configuration applies. This `condition` must be of type `CACHE`. For detailed information about Conditionals, see [Fastly's Documentation on Conditionals](https://docs.fastly.com/en/guides/using-conditions)
- **content_types** (Set of String) The content-type for each type of content you wish to have dynamically gzip'ed. Example: `["text/html", "text/css"]`
- **extensions** (Set of String) File extensions for each file type to dynamically gzip. Example: `["css", "js"]`
- **content_types** (List of String) The content-type for each type of content you wish to have dynamically gzip'ed. Example: `["text/html", "text/css"]`
- **extensions** (List of String) File extensions for each file type to dynamically gzip. Example: `["css", "js"]`


<a id="nestedblock--header"></a>
Expand Down
60 changes: 21 additions & 39 deletions fastly/block_fastly_service_v1_gzip.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,23 +78,11 @@ func (h *GzipServiceAttributeHandler) Process(d *schema.ResourceData, latestVers
}

if v, ok := resource["content_types"]; ok {
if len(v.(*schema.Set).List()) > 0 {
var cl []string
for _, c := range v.(*schema.Set).List() {
cl = append(cl, c.(string))
}
opts.ContentTypes = strings.Join(cl, " ")
}
opts.ContentTypes = sliceToString(v.([]interface{}))
}

if v, ok := resource["extensions"]; ok {
if len(v.(*schema.Set).List()) > 0 {
var el []string
for _, e := range v.(*schema.Set).List() {
el = append(el, e.(string))
}
opts.Extensions = strings.Join(el, " ")
}
opts.Extensions = sliceToString(v.([]interface{}))
}

log.Printf("[DEBUG] Fastly Gzip Addition opts: %#v", opts)
Expand All @@ -118,33 +106,19 @@ func (h *GzipServiceAttributeHandler) Process(d *schema.ResourceData, latestVers
Name: resource["name"].(string),
}

// NOTE: []interface{} is not comparable in Filter function
// convert it into string in advance
resource["content_types"] = sliceToString(resource["content_types"].([]interface{}))
resource["extensions"] = sliceToString(resource["extensions"].([]interface{}))

// only attempt to update attributes that have changed
modified := setDiff.Filter(resource, oldSet)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just thought of a possible issue here as well - you're updating resource[...] to be a string, but oldSet will still have it as []interface{} so will probably always be included in modified. It's a bit confusing but I don't think it causes any errors because it will just include the existing value in the update every time.

There's probably two options to rectify that: either just set resource["content_types"] etc to nil with a comment to explain that it doesn't matter if you redundantly update these values; or you could update setDiff.Filter to be more intelligent than just a simple != equality check (i.e. checking the types before comparing, and adding a better check for more complex values like slices)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of that pattern as well at first. But, I wasn't sure if that's a good idea to check the types for all inputs unconditionally. That seems a bit inefficient processing.

Like, we could make the Filter function take optional parameters (e.g., AFOP) and do some custom conversions/diff based on the block type, for instance. This way, Filter function is still backward-compatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about something like this? Avoids a panic by using DeepEqual which checks the types for you, and makes it clear that we're ignoring the resulting modified variable for content_types and extensions by always updating them. I haven't run the whole acceptance test suite to double check but seems like it would work?

diff --git a/fastly/block_fastly_service_v1_gzip.go b/fastly/block_fastly_service_v1_gzip.go
index 756a3cec..25813411 100644
--- a/fastly/block_fastly_service_v1_gzip.go
+++ b/fastly/block_fastly_service_v1_gzip.go
@@ -104,22 +104,13 @@ func (h *GzipServiceAttributeHandler) Process(d *schema.ResourceData, latestVers
 			ServiceID:      d.Id(),
 			ServiceVersion: latestVersion,
 			Name:           resource["name"].(string),
+			ContentTypes:   gofastly.String(sliceToString(resource["content_types"].([]interface{}))),
+			Extensions:     gofastly.String(sliceToString(resource["extensions"].([]interface{}))),
 		}
 
-		// NOTE: []interface{} is not comparable in Filter function
-		// convert it into string in advance
-		resource["content_types"] = sliceToString(resource["content_types"].([]interface{}))
-		resource["extensions"] = sliceToString(resource["extensions"].([]interface{}))
-
 		// only attempt to update attributes that have changed
 		modified := setDiff.Filter(resource, oldSet)
 
-		if v, ok := modified["content_types"]; ok {
-			opts.ContentTypes = gofastly.String(v.(string))
-		}
-		if v, ok := modified["extensions"]; ok {
-			opts.Extensions = gofastly.String(v.(string))
-		}
 		if v, ok := modified["cache_condition"]; ok {
 			opts.CacheCondition = gofastly.String(v.(string))
 		}
diff --git a/fastly/diff.go b/fastly/diff.go
index c337c065..8151af66 100644
--- a/fastly/diff.go
+++ b/fastly/diff.go
@@ -2,6 +2,7 @@ package fastly
 
 import (
 	"fmt"
+	"reflect"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 )
@@ -132,7 +133,7 @@ func (h *SetDiff) Filter(modified map[string]interface{}, oldSet *schema.Set) ma
 
 		if m["name"].(string) == modified["name"].(string) {
 			for k, v := range m {
-				if v != modified[k] {
+				if !reflect.DeepEqual(v, modified[k]) {
 					filtered[k] = modified[k]
 				}
 			}


// NOTE: where we transition between interface{} we lose the ability to
// infer the underlying type being either a uint vs an int. This
// materializes as a panic (yay) and so it's only at runtime we discover
// this and so we've updated the below code to convert the type asserted
// int into a uint before passing the value to gofastly.Uint().
if v, ok := modified["content_types"]; ok {
set := v.(*schema.Set)
if len(set.List()) > 0 {
var s []string
for _, elem := range set.List() {
s = append(s, elem.(string))
}
opts.ContentTypes = gofastly.String(strings.Join(s, " "))
}
opts.ContentTypes = gofastly.String(v.(string))
}
if v, ok := modified["extensions"]; ok {
set := v.(*schema.Set)
if len(set.List()) > 0 {
var s []string
for _, elem := range set.List() {
s = append(s, elem.(string))
}
opts.Extensions = gofastly.String(strings.Join(s, " "))
}
opts.Extensions = gofastly.String(v.(string))
}
if v, ok := modified["cache_condition"]; ok {
opts.CacheCondition = gofastly.String(v.(string))
Expand Down Expand Up @@ -194,13 +168,13 @@ func (h *GzipServiceAttributeHandler) Register(s *schema.Resource) error {
},
// optional fields
"content_types": {
Type: schema.TypeSet,
Type: schema.TypeList,
Optional: true,
Description: "The content-type for each type of content you wish to have dynamically gzip'ed. Example: `[\"text/html\", \"text/css\"]`",
Elem: &schema.Schema{Type: schema.TypeString},
},
"extensions": {
Type: schema.TypeSet,
Type: schema.TypeList,
Optional: true,
Description: "File extensions for each file type to dynamically gzip. Example: `[\"css\", \"js\"]`",
Elem: &schema.Schema{Type: schema.TypeString},
Expand Down Expand Up @@ -232,7 +206,7 @@ func flattenGzips(gzipsList []*gofastly.Gzip) []map[string]interface{} {
for _, ev := range e {
et = append(et, ev)
}
ng["extensions"] = schema.NewSet(schema.HashString, et)
ng["extensions"] = et
}

if g.ContentTypes != "" {
Expand All @@ -241,7 +215,7 @@ func flattenGzips(gzipsList []*gofastly.Gzip) []map[string]interface{} {
for _, cv := range c {
ct = append(ct, cv)
}
ng["content_types"] = schema.NewSet(schema.HashString, ct)
ng["content_types"] = ct
}

// prune any empty values that come from the default string value in structs
Expand All @@ -256,3 +230,11 @@ func flattenGzips(gzipsList []*gofastly.Gzip) []map[string]interface{} {

return gl
}

func sliceToString(src []interface{}) string {
var result []string
for _, el := range src {
result = append(result, el.(string))
}
return strings.Join(result, " ")
}
69 changes: 16 additions & 53 deletions fastly/block_fastly_service_v1_gzip_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
gofastly "github.com/fastly/go-fastly/v3/fastly"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
)

Expand All @@ -27,7 +26,7 @@ func TestResourceFastlyFlattenGzips(t *testing.T) {
local: []map[string]interface{}{
{
"name": "somegzip",
"extensions": schema.NewSet(schema.HashString, []interface{}{"css"}),
"extensions": []interface{}{"css"},
},
},
},
Expand All @@ -47,57 +46,22 @@ func TestResourceFastlyFlattenGzips(t *testing.T) {
local: []map[string]interface{}{
{
"name": "somegzip",
"extensions": schema.NewSet(schema.HashString, []interface{}{"css", "json", "js"}),
"content_types": schema.NewSet(schema.HashString, []interface{}{"text/html"}),
"extensions": []interface{}{"css", "json", "js"},
"content_types": []interface{}{"text/html"},
},
{
"name": "someothergzip",
"extensions": schema.NewSet(schema.HashString, []interface{}{"css", "js"}),
"content_types": schema.NewSet(schema.HashString, []interface{}{"text/html", "text/xml"}),
"extensions": []interface{}{"css", "js"},
"content_types": []interface{}{"text/html", "text/xml"},
},
},
},
}

for _, c := range cases {
out := flattenGzips(c.remote)
// loop, because deepequal wont work with our sets
expectedCount := len(c.local)
var found int
for _, o := range out {
for _, l := range c.local {
if o["name"].(string) == l["name"].(string) {
found++
if o["extensions"] == nil && l["extensions"] != nil {
t.Fatalf("output extensions are nil, local are not")
}

if o["extensions"] != nil {
oex := o["extensions"].(*schema.Set)
lex := l["extensions"].(*schema.Set)
if !oex.Equal(lex) {
t.Fatalf("Extensions don't match, expected: %#v, got: %#v", lex, oex)
}
}

if o["content_types"] == nil && l["content_types"] != nil {
t.Fatalf("output content types are nil, local are not")
}

if o["content_types"] != nil {
oct := o["content_types"].(*schema.Set)
lct := l["content_types"].(*schema.Set)
if !oct.Equal(lct) {
t.Fatalf("ContentTypes don't match, expected: %#v, got: %#v", lct, oct)
}
}

}
}
}

if found != expectedCount {
t.Fatalf("Found and expected mismatch: %d / %d", found, expectedCount)
if !reflect.DeepEqual(out, c.local) {
t.Fatalf("Error matching:\nexpected: %#v\ngot: %#v", c.local, out)
}
}
}
Expand All @@ -110,7 +74,7 @@ func TestAccFastlyServiceV1_gzips_basic(t *testing.T) {
log1 := gofastly.Gzip{
ServiceVersion: 1,
Name: "gzip file types",
Extensions: "js css",
Extensions: "css js",
CacheCondition: "testing_condition",
}

Expand All @@ -123,15 +87,15 @@ func TestAccFastlyServiceV1_gzips_basic(t *testing.T) {
log3 := gofastly.Gzip{
ServiceVersion: 1,
Name: "all",
Extensions: "js html css",
Extensions: "css js html",
ContentTypes: "text/javascript application/x-javascript application/javascript text/css text/html",
}

log4 := gofastly.Gzip{
ServiceVersion: 1,
Name: "all",
Extensions: "css",
ContentTypes: "text/javascript application/x-javascript",
ContentTypes: "application/x-javascript text/javascript",
}

resource.ParallelTest(t, resource.TestCase{
Expand Down Expand Up @@ -252,7 +216,7 @@ resource "fastly_service_v1" "foo" {

gzip {
name = "gzip extensions"
content_types = ["text/html", "text/css"]
content_types = ["text/css", "text/html"]
}

force_destroy = true
Expand All @@ -279,12 +243,11 @@ resource "fastly_service_v1" "foo" {
extensions = ["css", "js", "html"]

content_types = [
"text/html",
"text/css",
"application/x-javascript",
"text/css",
"application/javascript",
"text/javascript",
"text/javascript",
"application/x-javascript",
"application/javascript",
"text/css",
"text/html",
]
}

Expand Down