validation: add additional Int, Network, Time and Web validators #296

katbyte · 2020-01-15T06:51:04Z

branched off #295 so diff should clean up once its merged

New Validators:

IntDivisibleBy
IntNotInSlice
IsIPv6Address
IsIPv4Address
IsCIDR
IsMACAddress
IsPortNumber
IsPortNumberOrZero
IsDayOfTheWeek
IsMonth
IsRFC3339Time
IsURLWithHTTPS
IsURLWithHTTPorHTTPS
IsURLWithScheme

radeksimko

I will let you rebase this before fully reviewing, but I thought it would be good to bring this up early.

I think ideally the function names should communicate whether it's a positive or negative validation. I understand that negated validation doesn't make sense in all cases, but it would be great to have some consistency and clarity.

for most, or at least these I'd consider using Is, Valid, or IsValid prefix

IPv6Address
IPv4Address
CIDR
MACAddress
PortNumber
PortNumberOrZero
URLWithScheme

Also it would be great to list all the validation functions being added, so we can paste these into the Changelog - I noticed there's a few more (e.g. SingleIP, IPRange).

FYI: I also created #298 to spin off naming convention as a parallel topic.

katbyte · 2020-01-15T13:07:11Z

Happy to make those changes @radeksimko, however SingleIP, IPRange already existed in the SDK. Once this is merged i will be opening a PR that deprecates and noralizes the remaining validators that match 🙂

radeksimko · 2020-01-15T13:16:34Z

I see, apologies - it wasn't clear from the diff 🙃

katbyte · 2020-01-15T13:18:37Z

Renamed! & ready for review

katbyte · 2020-01-15T13:21:02Z

I will also say i am torn with functions like StringIsRFC3339Time and StringIsDayOfWeek, i almost want to just have them be IsDayOfWeek, IsRFC3339Time? but not sure if that is a good idea or not.

appilon · 2020-01-15T19:39:00Z

@katbyte I personally prefer the String prefix, to me it implies first and foremost we will typecheck and error if the type was not a string.

appilon · 2020-01-15T19:42:32Z

helper/validation/network.go

 		return
 	}
+
+	if v < 0 || 65535 < v {


nitpick, prefer the variable to always be on the left side of the comparison

if v < 0 || v > 65535 { ... }

appilon · 2020-01-15T19:48:02Z

helper/validation/int.go

@@ -83,3 +103,23 @@ func IntInSlice(valid []int) schema.SchemaValidateFunc {
 		return
 	}
 }
+
+// IntInSlice returns a SchemaValidateFunc which tests if the provided value


comment/doc needs updating

appilon · 2020-01-15T19:49:00Z

helper/validation/network.go

-				"expected %s to contain a valid network CIDR, expected %s, got %s",
-				k, ipnet, v))
-		}
+func IsIPv6Address(i interface{}, k string) (warnings []string, errors []error) {


Needs doc/comment

appilon · 2020-01-15T19:49:10Z

helper/validation/network.go

+	return warnings, errors
+}
+
+func IsIPv4Address(i interface{}, k string) (warnings []string, errors []error) {


Needs doc/comment

appilon · 2020-01-15T19:49:54Z

helper/validation/network.go

-// IPRange returns a SchemaValidateFunc which tests if the provided value
-// is of type string, and in valid IP range notation
-func IPRange() schema.SchemaValidateFunc {
+func IsCIDR(i interface{}, k string) (warnings []string, errors []error) {


Needs doc/comment

appilon · 2020-01-15T19:52:51Z

helper/validation/time.go

-func ValidateRFC3339TimeString(v interface{}, k string) (ws []string, errors []error) {
-	if _, err := time.Parse(time.RFC3339, v.(string)); err != nil {
-		errors = append(errors, fmt.Errorf("%q: invalid RFC3339 timestamp", k))
+func IsDayOfTheWeek(ignoreCase bool) schema.SchemaValidateFunc {


Needs doc/comment

appilon · 2020-01-15T19:52:59Z

helper/validation/time.go

+	}, ignoreCase)
+}
+
+func IsMonth(ignoreCase bool) schema.SchemaValidateFunc {


Needs doc/comment

appilon · 2020-01-15T19:53:16Z

helper/validation/time.go

+}
+
+// ValidateRFC3339TimeString is a ValidateFunc that ensures a string parses as time.RFC3339 format
+func IsRFC3339Time(i interface{}, k string) (warnings []string, errors []error) {


Comment needs update

appilon · 2020-01-15T19:54:12Z

helper/validation/web.go

+	return IsURLWithScheme([]string{"http", "https"})(i, k)
+}
+
+func IsURLWithScheme(validSchemes []string) schema.SchemaValidateFunc {


Needs doc/comment

appilon · 2020-01-15T19:55:25Z

helper/validation/web.go

+	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+)
+
+func URLIsHTTPS(i interface{}, k string) (_ []string, errors []error) {


I like the way this flows... but it feels inconsistent, anyway this can be reworded to being with Is... maybe

IsHTTPSURL

and

IsHTTPURL

appilon · 2020-01-16T17:10:43Z

helper/validation/web_test.go

+	"testing"
+)
+
+func TestURLIsHTTPS(t *testing.T) {


just need to update Test names

appilon · 2020-01-16T17:17:29Z

helper/validation/web.go

+	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
+)
+
+// IsURLWithHTTPS is a SchemaValidateFunc which tests if the provided value is of type string and a valid IPv6 address


Ah dang looks like doc/comments need description update

ghost · 2020-02-16T01:50:01Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

katbyte added 4 commits January 14, 2020 15:26

sort validation function into separate files

3f3dde2

cleanup

9a86338

make fmt

ee4fe2a

validation: add Int, Network, Time and Web validators

c1005d6

katbyte changed the title ~~validation: add Int, Network, Time and Web validators~~ validation: add additional Int, Network, Time and Web validators Jan 15, 2020

Fix errant find replace

af70e5e

radeksimko mentioned this pull request Jan 15, 2020

Set a naming convention for validation functions #298

Closed

radeksimko reviewed Jan 15, 2020

View reviewed changes

git merge master -S

197bcb9

Added Is to function names

2504e75

Removed String from network and time functions

43d0e8f

appilon suggested changes Jan 15, 2020

View reviewed changes

katbyte added 4 commits January 16, 2020 08:44

Address PR comments

d00d319

make fmt

c4e0f07

Further PR comments

03bfef2

adjust max port value

f86cf8d

appilon suggested changes Jan 16, 2020

View reviewed changes

helper/validation/web_test.go Outdated

"testing"

)

func TestURLIsHTTPS(t *testing.T) {

Copy link

Contributor

appilon Jan 16, 2020

Choose a reason for hiding this comment

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

just need to update Test names

rename tests

e481cb9

appilon suggested changes Jan 16, 2020

View reviewed changes

katbyte added 2 commits January 16, 2020 09:24

Update web.go

4770c6a

Update web.go

cc4d279

appilon merged commit 5cc4681 into master Jan 16, 2020

appilon deleted the kt/azurerm-validators-2 branch January 16, 2020 17:33

katbyte mentioned this pull request Jan 21, 2020

Fix for Issue #5227 hashicorp/terraform-provider-azurerm#5461

Closed

ghost locked and limited conversation to collaborators Feb 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validation: add additional Int, Network, Time and Web validators #296

validation: add additional Int, Network, Time and Web validators #296

katbyte commented Jan 15, 2020 •

edited by appilon

Loading

radeksimko left a comment

katbyte commented Jan 15, 2020

radeksimko commented Jan 15, 2020

katbyte commented Jan 15, 2020

katbyte commented Jan 15, 2020

appilon commented Jan 15, 2020

appilon Jan 15, 2020

appilon Jan 15, 2020

appilon Jan 15, 2020

appilon Jan 15, 2020

appilon Jan 15, 2020

appilon Jan 15, 2020

appilon Jan 15, 2020

appilon Jan 15, 2020

appilon Jan 15, 2020

appilon Jan 15, 2020

appilon Jan 16, 2020

appilon Jan 16, 2020

ghost commented Feb 16, 2020

validation: add additional Int, Network, Time and Web validators #296

validation: add additional Int, Network, Time and Web validators #296

Conversation

katbyte commented Jan 15, 2020 • edited by appilon Loading

radeksimko left a comment

Choose a reason for hiding this comment

katbyte commented Jan 15, 2020

radeksimko commented Jan 15, 2020

katbyte commented Jan 15, 2020

katbyte commented Jan 15, 2020

appilon commented Jan 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 16, 2020

katbyte commented Jan 15, 2020 •

edited by appilon

Loading