From cfcfacd59e54d156d3a7694a364bbfc21d4a8ff6 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 10 Aug 2023 13:05:17 -0400 Subject: [PATCH 1/5] emr/cluster: Allow bootstrap action empty args --- internal/flex/flex.go | 16 +++++++++++++++- internal/service/emr/cluster.go | 2 +- internal/service/emr/cluster_test.go | 12 ++++++------ 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/internal/flex/flex.go b/internal/flex/flex.go index 43005cd29db7..ab0abe6e3ed7 100644 --- a/internal/flex/flex.go +++ b/internal/flex/flex.go @@ -23,13 +23,27 @@ const ( func ExpandStringList(configured []interface{}) []*string { vs := make([]*string, 0, len(configured)) for _, v := range configured { - if v, ok := v.(string); ok && v != "" { + if v, ok := v.(string); ok && v != "" { // v != "" is not necessary here, "" = !ok (weird Go-ism) vs = append(vs, aws.String(v)) } } return vs } +// ExpandStringListEmpty the result of flatmap. Expand for an array of strings +// and returns a []*string. Empty strings are NOT skipped. +func ExpandStringListEmpty(configured []interface{}) []*string { + vs := make([]*string, 0, len(configured)) + for _, v := range configured { + if v, ok := v.(string); ok { // empty string is !ok + vs = append(vs, aws.String(v)) + } else { + vs = append(vs, aws.String("")) + } + } + return vs +} + // Takes the result of flatmap.Expand for an array of strings // and returns a []*time.Time func ExpandStringTimeList(configured []interface{}, format string) []*time.Time { diff --git a/internal/service/emr/cluster.go b/internal/service/emr/cluster.go index faac7a6412fe..5cd02c2ae826 100644 --- a/internal/service/emr/cluster.go +++ b/internal/service/emr/cluster.go @@ -1740,7 +1740,7 @@ func expandBootstrapActions(bootstrapActions []interface{}) []*emr.BootstrapActi Name: aws.String(actionName), ScriptBootstrapAction: &emr.ScriptBootstrapActionConfig{ Path: aws.String(actionPath), - Args: flex.ExpandStringList(actionArgs), + Args: flex.ExpandStringListEmpty(actionArgs), }, } actionsOut = append(actionsOut, action) diff --git a/internal/service/emr/cluster_test.go b/internal/service/emr/cluster_test.go index 8ee02c870275..2a0434db5194 100644 --- a/internal/service/emr/cluster_test.go +++ b/internal/service/emr/cluster_test.go @@ -1043,7 +1043,7 @@ func TestAccEMRCluster_Bootstrap_ordering(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "bootstrap_action.1.args.#", "11"), resource.TestCheckResourceAttr(resourceName, "bootstrap_action.1.args.0", "0"), resource.TestCheckResourceAttr(resourceName, "bootstrap_action.1.args.1", "1"), - resource.TestCheckResourceAttr(resourceName, "bootstrap_action.1.args.2", "\"\""), + resource.TestCheckResourceAttr(resourceName, "bootstrap_action.1.args.2", ""), resource.TestCheckResourceAttr(resourceName, "bootstrap_action.1.args.3", "3"), resource.TestCheckResourceAttr(resourceName, "bootstrap_action.1.args.4", "4"), resource.TestCheckResourceAttr(resourceName, "bootstrap_action.1.args.5", "5"), @@ -1079,7 +1079,7 @@ func TestAccEMRCluster_Bootstrap_ordering(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "bootstrap_action.1.args.#", "11"), resource.TestCheckResourceAttr(resourceName, "bootstrap_action.1.args.0", "0"), resource.TestCheckResourceAttr(resourceName, "bootstrap_action.1.args.1", "1"), - resource.TestCheckResourceAttr(resourceName, "bootstrap_action.1.args.2", "\"\""), + resource.TestCheckResourceAttr(resourceName, "bootstrap_action.1.args.2", ""), resource.TestCheckResourceAttr(resourceName, "bootstrap_action.1.args.3", "3"), resource.TestCheckResourceAttr(resourceName, "bootstrap_action.1.args.4", "4"), resource.TestCheckResourceAttr(resourceName, "bootstrap_action.1.args.5", "5"), @@ -1120,7 +1120,7 @@ func TestAccEMRCluster_Bootstrap_ordering(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "bootstrap_action.2.args.#", "11"), resource.TestCheckResourceAttr(resourceName, "bootstrap_action.2.args.0", "0"), resource.TestCheckResourceAttr(resourceName, "bootstrap_action.2.args.1", "1"), - resource.TestCheckResourceAttr(resourceName, "bootstrap_action.2.args.2", "\"\""), + resource.TestCheckResourceAttr(resourceName, "bootstrap_action.2.args.2", ""), resource.TestCheckResourceAttr(resourceName, "bootstrap_action.2.args.3", "3"), resource.TestCheckResourceAttr(resourceName, "bootstrap_action.2.args.4", "4"), resource.TestCheckResourceAttr(resourceName, "bootstrap_action.2.args.5", "5"), @@ -3115,7 +3115,7 @@ resource "aws_emr_cluster" "test" { args = [ "0", "1", - "\"\"", + "", "3", "4", "5", @@ -3181,7 +3181,7 @@ resource "aws_emr_cluster" "test" { args = [ "0", "1", - "\"\"", + "", "3", "4", "5", @@ -3259,7 +3259,7 @@ resource "aws_emr_cluster" "test" { args = [ "0", "1", - "\"\"", + "", "3", "4", "5", From dc4cc4f6c6e7b834de19ba7ee829d3ea2f827e9e Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 10 Aug 2023 13:08:32 -0400 Subject: [PATCH 2/5] Add changelog --- .changelog/32956.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/32956.txt diff --git a/.changelog/32956.txt b/.changelog/32956.txt new file mode 100644 index 000000000000..8bb5ce0fee43 --- /dev/null +++ b/.changelog/32956.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_emr_cluster: Fix to allow empty `args` for `bootstrap_action` +``` \ No newline at end of file From 06d680ed8ea58fdbbdf4bf514f61c01a976252cc Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 10 Aug 2023 14:33:20 -0400 Subject: [PATCH 3/5] emr/cluster: Improve comments --- internal/flex/flex.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/flex/flex.go b/internal/flex/flex.go index ab0abe6e3ed7..5d4f38a7dd7b 100644 --- a/internal/flex/flex.go +++ b/internal/flex/flex.go @@ -23,7 +23,7 @@ const ( func ExpandStringList(configured []interface{}) []*string { vs := make([]*string, 0, len(configured)) for _, v := range configured { - if v, ok := v.(string); ok && v != "" { // v != "" is not necessary here, "" = !ok (weird Go-ism) + if v, ok := v.(string); ok && v != "" { vs = append(vs, aws.String(v)) } } @@ -35,7 +35,7 @@ func ExpandStringList(configured []interface{}) []*string { func ExpandStringListEmpty(configured []interface{}) []*string { vs := make([]*string, 0, len(configured)) for _, v := range configured { - if v, ok := v.(string); ok { // empty string is !ok + if v, ok := v.(string); ok { // empty string in config turns into nil in []interface{} so !ok vs = append(vs, aws.String(v)) } else { vs = append(vs, aws.String("")) From b31ebca4da7f4dd4abe2ad842bf762fea24b8be6 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 10 Aug 2023 14:39:51 -0400 Subject: [PATCH 4/5] Fix comment --- internal/flex/flex.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/flex/flex.go b/internal/flex/flex.go index 5d4f38a7dd7b..20137e701c8a 100644 --- a/internal/flex/flex.go +++ b/internal/flex/flex.go @@ -23,7 +23,7 @@ const ( func ExpandStringList(configured []interface{}) []*string { vs := make([]*string, 0, len(configured)) for _, v := range configured { - if v, ok := v.(string); ok && v != "" { + if v, ok := v.(string); ok && v != "" { // v != "" may not do anything since in []interface{}, empty string will be nil so !ok vs = append(vs, aws.String(v)) } } From 2dc1edbcbc88ac10679be3f02bcd09d7bfdbe061 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 10 Aug 2023 15:24:44 -0400 Subject: [PATCH 5/5] Unit tests, go mod tidy --- internal/flex/flex.go | 2 +- internal/flex/flex_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/internal/flex/flex.go b/internal/flex/flex.go index 20137e701c8a..4ade8c6dd049 100644 --- a/internal/flex/flex.go +++ b/internal/flex/flex.go @@ -31,7 +31,7 @@ func ExpandStringList(configured []interface{}) []*string { } // ExpandStringListEmpty the result of flatmap. Expand for an array of strings -// and returns a []*string. Empty strings are NOT skipped. +// and returns a []*string. Adds an empty element for every nil or uncastable. func ExpandStringListEmpty(configured []interface{}) []*string { vs := make([]*string, 0, len(configured)) for _, v := range configured { diff --git a/internal/flex/flex_test.go b/internal/flex/flex_test.go index 5f24939207ea..94ec3aa21b8b 100644 --- a/internal/flex/flex_test.go +++ b/internal/flex/flex_test.go @@ -39,6 +39,37 @@ func TestExpandStringList(t *testing.T) { } } +func TestExpandStringListEmpty(t *testing.T) { + t.Parallel() + + testCases := []struct { + configured []interface{} + want []*string + }{ + { + configured: []interface{}{"abc", "xyz123"}, + want: []*string{aws.String("abc"), aws.String("xyz123")}, + }, + { + configured: []interface{}{"abc", 123, "xyz123"}, + want: []*string{aws.String("abc"), aws.String(""), aws.String("xyz123")}, + }, + { + configured: []interface{}{"foo", "bar", "", "baz"}, + want: []*string{aws.String("foo"), aws.String("bar"), aws.String(""), aws.String("baz")}, + }, + { + configured: []interface{}{"foo", "bar", nil, "baz"}, + want: []*string{aws.String("foo"), aws.String("bar"), aws.String(""), aws.String("baz")}, + }, + } + for _, testCase := range testCases { + if got, want := ExpandStringListEmpty(testCase.configured), testCase.want; !cmp.Equal(got, want) { + t.Errorf("ExpandStringListEmpty(%v) = %v, want %v", testCase.configured, got, want) + } + } +} + func TestExpandStringTimeList(t *testing.T) { t.Parallel()