From 6fbb918f1361828e54399c91ec84b9b0216c8884 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 26 May 2023 16:23:48 +0200 Subject: [PATCH 1/5] WIP --- provider/parameter.go | 27 ++++++++++++--------------- provider/parameter_test.go | 26 +++++++++++++++----------- 2 files changed, 27 insertions(+), 26 deletions(-) diff --git a/provider/parameter.go b/provider/parameter.go index 972950a6..2b083f63 100644 --- a/provider/parameter.go +++ b/provider/parameter.go @@ -26,8 +26,8 @@ type Option struct { } type Validation struct { - Min int - Max int + Min *int + Max *int Monotonic string Regex string @@ -272,17 +272,14 @@ func parameterDataSource() *schema.Resource { Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "min": { - Type: schema.TypeInt, - Optional: true, - Default: 0, - Description: "The minimum of a number parameter.", - RequiredWith: []string{"validation.0.max"}, + Type: schema.TypeInt, + Optional: true, + Description: "The minimum of a number parameter.", }, "max": { - Type: schema.TypeInt, - Optional: true, - Description: "The maximum of a number parameter.", - RequiredWith: []string{"validation.0.min"}, + Type: schema.TypeInt, + Optional: true, + Description: "The maximum of a number parameter.", }, "monotonic": { Type: schema.TypeString, @@ -353,10 +350,10 @@ func valueIsType(typ, value string) diag.Diagnostics { func (v *Validation) Valid(typ, value string) error { if typ != "number" { - if v.Min != 0 { + if v.Min != nil { return fmt.Errorf("a min cannot be specified for a %s type", typ) } - if v.Max != 0 { + if v.Max != nil { return fmt.Errorf("a max cannot be specified for a %s type", typ) } } @@ -389,10 +386,10 @@ func (v *Validation) Valid(typ, value string) error { if err != nil { return fmt.Errorf("value %q is not a number", value) } - if num < v.Min { + if v.Min != nil && num < *v.Min { return fmt.Errorf("value %d is less than the minimum %d", num, v.Min) } - if num > v.Max { + if v.Max != nil && num > *v.Max { return fmt.Errorf("value %d is more than the maximum %d", num, v.Max) } if v.Monotonic != "" && v.Monotonic != ValidationMonotonicIncreasing && v.Monotonic != ValidationMonotonicDecreasing { diff --git a/provider/parameter_test.go b/provider/parameter_test.go index c349190b..be261b8d 100644 --- a/provider/parameter_test.go +++ b/provider/parameter_test.go @@ -443,18 +443,18 @@ func TestValueValidatesType(t *testing.T) { Regex, RegexError string Min, - Max int + Max *int Monotonic string Error *regexp.Regexp }{{ Name: "StringWithMin", Type: "string", - Min: 1, + Min: ptrNumber(1), Error: regexp.MustCompile("cannot be specified"), }, { Name: "StringWithMax", Type: "string", - Max: 1, + Max: ptrNumber(1), Error: regexp.MustCompile("cannot be specified"), }, { Name: "NonStringWithRegex", @@ -474,13 +474,13 @@ func TestValueValidatesType(t *testing.T) { Name: "NumberBelowMin", Type: "number", Value: "0", - Min: 1, + Min: ptrNumber(1), Error: regexp.MustCompile("is less than the minimum"), }, { Name: "NumberAboveMax", Type: "number", Value: "1", - Max: 0, + Max: ptrNumber(1), Error: regexp.MustCompile("is more than the maximum"), }, { Name: "InvalidBool", @@ -498,23 +498,23 @@ func TestValueValidatesType(t *testing.T) { Name: "InvalidMonotonicity", Type: "number", Value: "1", - Min: 0, - Max: 2, + Min: ptrNumber(0), + Max: ptrNumber(2), Monotonic: "foobar", Error: regexp.MustCompile(`number monotonicity can be either "increasing" or "decreasing"`), }, { Name: "IncreasingMonotonicity", Type: "number", Value: "1", - Min: 0, - Max: 2, + Min: ptrNumber(0), + Max: ptrNumber(2), Monotonic: "increasing", }, { Name: "DecreasingMonotonicity", Type: "number", Value: "1", - Min: 0, - Max: 2, + Min: ptrNumber(0), + Max: ptrNumber(2), Monotonic: "decreasing", }, { Name: "ValidListOfStrings", @@ -550,3 +550,7 @@ func TestValueValidatesType(t *testing.T) { }) } } + +func ptrNumber(i int) *int { + return &i +} From 5ee802d79bdae2b9737a8351c283ea69f8731fe5 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 26 May 2023 18:09:35 +0200 Subject: [PATCH 2/5] fix: coder_parameter validation min and max are truly optional --- provider/parameter.go | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/provider/parameter.go b/provider/parameter.go index 2b083f63..263022a5 100644 --- a/provider/parameter.go +++ b/provider/parameter.go @@ -62,8 +62,25 @@ func parameterDataSource() *schema.Resource { ReadContext: func(ctx context.Context, rd *schema.ResourceData, i interface{}) diag.Diagnostics { rd.SetId(uuid.NewString()) + // 1. Read raw config to check if ptr fields are nil + // 2. Update rd with nil values (it is already broken) + + //vm := rd.GetRawConfig().AsValueMap()["validation"].AsValueSlice()[0].AsValueMap() + //log.Println(vm) + + val := rd.Get("validation") + v := val.([]interface{}) + k := v[0].(map[string]interface{}) + k["min"] = nil + k["max"] = nil + v[0] = k + err := rd.Set("validation", v) + if err != nil { + return diag.FromErr(err) + } + var parameter Parameter - err := mapstructure.Decode(struct { + err = mapstructure.Decode(struct { Value interface{} Name interface{} DisplayName interface{} @@ -98,7 +115,7 @@ func parameterDataSource() *schema.Resource { }(), Icon: rd.Get("icon"), Option: rd.Get("option"), - Validation: rd.Get("validation"), + Validation: val, Optional: func() bool { // This hack allows for checking if the "default" field is present in the .tf file. // If "default" is missing or is "null", then it means that this field is required, From bcd69e78aed84f555a00b32aa8f7c30008040995 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 29 May 2023 12:54:00 +0200 Subject: [PATCH 3/5] implementation --- provider/decode_test.go | 17 +++++++++-- provider/parameter.go | 62 ++++++++++++++++++++++++++++++-------- provider/parameter_test.go | 24 +++++++++++++++ 3 files changed, 87 insertions(+), 16 deletions(-) diff --git a/provider/decode_test.go b/provider/decode_test.go index c7215bd8..ca3feccd 100644 --- a/provider/decode_test.go +++ b/provider/decode_test.go @@ -5,6 +5,7 @@ import ( "github.com/coder/terraform-provider-coder/provider" "github.com/mitchellh/mapstructure" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -18,15 +19,25 @@ func TestDecode(t *testing.T) { aMap := map[string]interface{}{ "name": "Parameter Name", + "type": "number", "display_name": displayName, "legacy_variable": legacyVariable, "legacy_variable_name": legacyVariableName, + "min": nil, + "validation": []map[string]interface{}{ + { + "min": nil, + "max": 5, + }, + }, } var param provider.Parameter err := mapstructure.Decode(aMap, ¶m) require.NoError(t, err) - require.Equal(t, displayName, param.DisplayName) - require.Equal(t, legacyVariable, param.LegacyVariable) - require.Equal(t, legacyVariableName, param.LegacyVariableName) + assert.Equal(t, displayName, param.DisplayName) + assert.Equal(t, legacyVariable, param.LegacyVariable) + assert.Equal(t, legacyVariableName, param.LegacyVariableName) + assert.Equal(t, (*int)(nil), param.Validation[0].Min) + assert.Equal(t, 5, *param.Validation[0].Max) } diff --git a/provider/parameter.go b/provider/parameter.go index 263022a5..09afba11 100644 --- a/provider/parameter.go +++ b/provider/parameter.go @@ -12,10 +12,12 @@ import ( "strconv" "github.com/google/uuid" + "github.com/hashicorp/go-cty/cty" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/mitchellh/mapstructure" + "golang.org/x/xerrors" ) type Option struct { @@ -62,19 +64,12 @@ func parameterDataSource() *schema.Resource { ReadContext: func(ctx context.Context, rd *schema.ResourceData, i interface{}) diag.Diagnostics { rd.SetId(uuid.NewString()) - // 1. Read raw config to check if ptr fields are nil - // 2. Update rd with nil values (it is already broken) - - //vm := rd.GetRawConfig().AsValueMap()["validation"].AsValueSlice()[0].AsValueMap() - //log.Println(vm) + fixedValidation, err := fixValidationResourceData(rd.GetRawConfig(), rd.Get("validation")) + if err != nil { + return diag.FromErr(err) + } - val := rd.Get("validation") - v := val.([]interface{}) - k := v[0].(map[string]interface{}) - k["min"] = nil - k["max"] = nil - v[0] = k - err := rd.Set("validation", v) + err = rd.Set("validation", fixedValidation) if err != nil { return diag.FromErr(err) } @@ -115,7 +110,7 @@ func parameterDataSource() *schema.Resource { }(), Icon: rd.Get("icon"), Option: rd.Get("option"), - Validation: val, + Validation: fixedValidation, Optional: func() bool { // This hack allows for checking if the "default" field is present in the .tf file. // If "default" is missing or is "null", then it means that this field is required, @@ -339,6 +334,47 @@ func parameterDataSource() *schema.Resource { } } +func fixValidationResourceData(rawConfig cty.Value, validation interface{}) (interface{}, error) { + // Read validation from raw config + rawValidation, ok := rawConfig.AsValueMap()["validation"] + if !ok { + return validation, nil // no validation rules, nothing to fix + } + + rawValidationArr := rawValidation.AsValueSlice() + if len(rawValidationArr) == 0 { + return validation, nil // no validation rules, nothing to fix + } + + rawValidationRule := rawValidationArr[0].AsValueMap() + + // Load validation from resource data + vArr, ok := validation.([]interface{}) + if !ok { + return nil, xerrors.New("validation should be an array") + } + + if len(vArr) == 0 { + return validation, nil // no validation rules, nothing to fix + } + + validationRule, ok := vArr[0].(map[string]interface{}) + if !ok { + return nil, xerrors.New("validation rule should be a map") + } + + // Fix the resource data + if rawValidationRule["min"].IsNull() { + validationRule["min"] = nil + } + if rawValidationRule["max"].IsNull() { + validationRule["max"] = nil + } + + vArr[0] = validationRule + return vArr, nil +} + func valueIsType(typ, value string) diag.Diagnostics { switch typ { case "number": diff --git a/provider/parameter_test.go b/provider/parameter_test.go index be261b8d..914059f4 100644 --- a/provider/parameter_test.go +++ b/provider/parameter_test.go @@ -109,6 +109,30 @@ func TestParameter(t *testing.T) { } } `, + }, { + Name: "NumberValidation_Min", + Config: ` + data "coder_parameter" "region" { + name = "Region" + type = "number" + default = 2 + validation { + min = 1 + } + } + `, + }, { + Name: "NumberValidation_Max", + Config: ` + data "coder_parameter" "region" { + name = "Region" + type = "number" + default = 2 + validation { + max = 9 + } + } + `, }, { Name: "DefaultNotNumber", Config: ` From 51e99519e34c3686bdbf9167cbd93ada4f0be92f Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 29 May 2023 12:59:08 +0200 Subject: [PATCH 4/5] fix --- provider/parameter_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/provider/parameter_test.go b/provider/parameter_test.go index 914059f4..4b2fe9c9 100644 --- a/provider/parameter_test.go +++ b/provider/parameter_test.go @@ -503,7 +503,7 @@ func TestValueValidatesType(t *testing.T) { }, { Name: "NumberAboveMax", Type: "number", - Value: "1", + Value: "2", Max: ptrNumber(1), Error: regexp.MustCompile("is more than the maximum"), }, { From a7959a09cb05141c81d7e82f40d91b9b523e41aa Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 29 May 2023 15:52:52 +0200 Subject: [PATCH 5/5] fix --- provider/parameter.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/provider/parameter.go b/provider/parameter.go index 09afba11..a9bbdc65 100644 --- a/provider/parameter.go +++ b/provider/parameter.go @@ -370,8 +370,6 @@ func fixValidationResourceData(rawConfig cty.Value, validation interface{}) (int if rawValidationRule["max"].IsNull() { validationRule["max"] = nil } - - vArr[0] = validationRule return vArr, nil }