add validate tag for the purpose of required field #886

rjtsdl · 2017-06-28T16:30:28Z

What this PR does / why we need it:
Currently in the versioned api definition, no way to know whether a field is required from user's input. The omitempty is mainly used for marshal. We need another tag for validation. By doing this, we could give user(the developer) a clear view of our api model definition without checking lines of validation code.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #884

Special notes for your reviewer:
I propose to add the tag. Check this
I picked this validator package https://github.com/go-playground/validator.

Release note:

This change is

rjtsdl · 2017-06-28T16:34:02Z

pkg/api/v20170701/types.go

@@ -18,7 +32,7 @@ type ResourcePurchasePlan struct {
 // resource definition in a JSON template.
 type ContainerService struct {
 	ID       string                `json:"id,omitempty"`


Please also help check if i missed any required field.

rjtsdl · 2017-06-28T16:57:37Z

@acs-bot test this please

JackQuincy · 2017-06-28T17:23:47Z

Any plans to enforce this in validation?

weinong · 2017-06-28T17:51:23Z

is it an experiment on 2017-07-01?

JiangtianLi · 2017-06-28T18:00:50Z

what go validator package do you plan to use? Is there anything more than "required/optional" we could add to tag?

rjtsdl · 2017-06-28T21:02:58Z

@weinong , could be an experiment.
@JackQuincy , yes, that's one of the next things i want to do. I mentioned it in the description. But i think it could be a different pr, if we think the validate tag makes sense.
@JiangtianLi , current i don't have more. It is all defined and handled by ourselves. Let me know if you have any idea.

amanohar · 2017-06-29T05:11:39Z

pkg/api/v20170701/types.go

 }

 // CustomProfile specifies custom properties that are used for
 // cluster instantiation.  Should not be used by most users.
 type CustomProfile struct {
-	Orchestrator string `json:"orchestrator,omitempty"`
+	Orchestrator string `json:"orchestrator,omitempty" validate:"required"`


Is this a required field?

I mark it as required, in the sense, CustomProfile is there.
However CustomProfile itself is not required.

Take a closer look, CustomProfile doesn't have any validate function. I don't want to add required here anymore.

amanohar · 2017-06-29T05:12:22Z

pkg/api/v20170701/types.go

-	Count                    int    `json:"count"`
-	DNSPrefix                string `json:"dnsPrefix"`
-	VMSize                   string `json:"vmSize"`
+	Count                    int    `json:"count" validate:"required"`


Doesn't master count have a default value? Same goes for VMSize, I.e. its not a required field

Master Count has a default value in the implementation of az cli.
You can think of az cli is the helper for user to put together the api model. This is the validation tag for this api model.

It works, after az cli generate the api model. We validate if it provide the required fields.
Keep in mind, this validate tag only works on validate the generated api model.

amanohar · 2017-06-29T05:16:09Z

pkg/api/v20170701/types.go

+// Current it is not used in any logic yet
+// TODO, we could possibly use this tag in all the Validate functions
+const (
+	ValidateTag = "validate"


Instead of calling it "validate" , why not "required" or "isrequired"?
E.g.:
Location string `json:"location,omitempty" isrequired:"true"

or

Location string `json:"location,omitempty" required:"true"

I will use whatever the validator package specified. My other comment has the options. Pick one that you favor more. They have different syntax.

rjtsdl · 2017-07-05T18:45:45Z

I have looked a few popular validator package.
I have listed them in the order of popularity. Any preference here?
@amanohar , after we choose the package, i will customize the tag in their way.
@JiangtianLi , which one you have tried? And yes, there is many more features, other than required and optional.

https://github.com/asaskevich/govalidator
https://github.com/go-playground/validator
https://github.com/go-validator/validator

JiangtianLi · 2017-07-05T18:56:18Z

@rjtsdl I haven't tried but just looked at its README. I think if the package can meet our container service validation, such as range, set, or even regex, that should be fine.

rjtsdl · 2017-07-05T20:06:12Z

I will pick this one. https://github.com/go-playground/validator
I will update this PR after apply this third party package

rjtsdl · 2017-07-05T21:23:46Z

@JackQuincy @amanohar @JiangtianLi @weinong , i have imported and applied https://github.com/go-playground/validator.v9. I don't expect any behavior change. I only did minimal change in v20170701/validate.go file. It proves the concept that, by introducing the package, things get clear and easier to do validation. We should be able to leverage this package further, when we get more and more familiar with it.

rjtsdl · 2017-07-05T21:30:57Z

pkg/api/v20170701/types.go

-	DNSPrefix                string `json:"dnsPrefix"`
-	VMSize                   string `json:"vmSize"`
-	OSDiskSizeGB             int    `json:"osDiskSizeGB,omitempty"`
+	Count                    int    `json:"count" validate:"required,eq=1|eq=3|eq=5"`


If we specify count with tags like this. It will be automatically validated. No need to write functions to handle it. TAL this file, and compare with what i have removed in the validate.go file. You will get the idea here.

i'm curious about the error message. any example?

Yes, when i specify count=2

FATA[0000] error parsing the api model: Key: 'MasterProfile.Count' Error:Field validation for 'Count' failed on the 'eq= 1|eq=3|eq=5' tag

Was that a fatal error log or a error message returned? if it is a fatal error log we can't use this framework

It is from the cmd below. The package itself doesn't panic or fatal.

./cmd/deploy.go: log.Fatalf("error parsing the api model: %s", err.Error()) ./cmd/generate.go: log.Fatalf("error parsing the api model: %s", err.Error()) ./cmd/upgrade.go: log.Fatalf("error parsing the api model: %s", err.Error())

the error msg at current form is not possible to be localized. So i'm strongly against doing this.

The returned error points out the struct/field/tag that failed so we could take that info than use it to generate a localized error message. But it would require us to make a bunch of if statements or a big switch case that works like an if else chain.

yes, that is exactly what i am doing. @JackQuincy , i should look at your comment earlier :(

@weinong , localization is handled now. It is not the blocker anymore. PTAL

The error message for the same MasterProfile.Count=2, would be like

FATA[0000] error parsing the api model: MasterProfile count needs to be 1, 3, or 5

seanknox · 2017-07-05T21:34:49Z

@rjtsdl From the end-user perspective, what changes?

rjtsdl · 2017-07-05T21:36:09Z

pkg/api/v20170701/validate.go

-	}
-	return nil
-}
-
 func validatePoolName(poolName string) error {
 	// we will cap at length of 12 and all lowercase letters since this makes up the VMName
 	poolNameRegex := `^([a-z][a-z0-9]{0,11})$`


We can take a look at regex later, when this package get more mature. This is the current state for regex.

regex a regex validator won't be added because commas and = signs can be part of a regex which conflict with the validation definitions. Although workarounds can be made, they take away from using pure regex's. Furthermore it's quick and dirty but the regex's become harder to maintain and are not reusable, so it's as much a programming philosiphy as anything. In place of this new validator functions should be created; a regex can be used within the validator function and even be precompiled for better efficiency within regexes.go. And the best reason, you can submit a pull request and we can keep on adding to the validation library of this package!

rjtsdl · 2017-07-05T21:45:36Z

@seanknox , Should be no behavior change for end user.
For developers, it would be clear API model declaration, and less customized validate functions.

amanohar · 2017-07-06T00:01:18Z

pkg/api/v20170701/types.go

 	DNSPrefix      string `json:"dnsPrefix"`
 	FQDN           string `json:"fqdn"`
 	Ports          []int  `json:"ports,omitempty"`
-	StorageProfile string `json:"storageProfile"`
+	StorageProfile string `json:"storageProfile" validate:"StorageAccount|ManagedDisks|len=0`
 	VnetSubnetID   string `json:"vnetSubnetID,omitempty"`


Add 'eq' condition to the tag

rjtsdl · 2017-07-07T03:45:49Z

pkg/api/v20170701/validate.go

 	}
 	return nil
 }

+func handleValidationErrors(e validator.ValidationErrors) error {


@JackQuincy @JiangtianLi @weinong
Now, this function will be called to handle the validator.ValidationErrors, it is called only in one place.
I noticed, I have a few hard coded strings here. However, it will be no difference with before, when we call

validateName(l.AdminUsername, "LinuxProfile.AdminUsername")

We also need to pass a hard coded string as a parameter.

This function is better in the sense, it has all hard-code strings in one place.

amanohar · 2017-07-07T18:29:14Z

Outside of the scope of this PR but discussed with @rjtsdl that some of the fields like count, vmSize that were not required before in the ACS service are not required. This impacts the model of asking the users for minimal set of inputs to stand up a cluster (an issue was opened to track discussion around this).

rjtsdl · 2017-07-07T18:30:23Z

@acs-bot test this please

weinong · 2017-07-07T18:31:17Z

Reviewed 1654 of 1656 files at r2, 1 of 2 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

pkg/api/v20170701/validate.go, line 105 at r5 (raw file):

Previously, rjtsdl (Jingtao Ren) wrote…

@JackQuincy @JiangtianLi @weinong
Now, this function will be called to handle the validator.ValidationErrors, it is called only in one place.
I noticed, I have a few hard coded strings here. However, it will be no difference with before, when we call
validateName(l.AdminUsername, "LinuxProfile.AdminUsername")
We also need to pass a hard coded string as a parameter.

This function is better in the sense, it has all hard-code strings in one place.

this is pretty cool

pkg/api/v20170701/validate.go, line 107 at r6 (raw file):

func handleValidationErrors(e validator.ValidationErrors) error {
	err := e[0]
	switch err.Namespace() {

err.Namespace() can be called once and re-used throughout this function.

Comments from Reviewable

rjtsdl · 2017-07-07T18:43:28Z

@weinong , address err.Namespace() suggestion. Thx.

Btw, don't know how to reply a comment from Reviewable :(

weinong · 2017-07-07T18:48:19Z

don't overwrite the commits please.

rjtsdl · 2017-07-07T18:52:49Z

Ah, sure, will add new commits.

rjtsdl · 2017-07-07T20:39:51Z

checked with everybody offline. I will merge this.

msftclas added the cla-not-required label Jun 28, 2017

rjtsdl requested review from anhowe and amanohar June 28, 2017 16:32

rjtsdl commented Jun 28, 2017

View reviewed changes

amanohar suggested changes Jun 29, 2017

View reviewed changes

rjtsdl force-pushed the jiren-addvalidatetag branch from 289fc38 to ea8e93c Compare July 5, 2017 21:13

rjtsdl commented Jul 5, 2017

View reviewed changes

rjtsdl force-pushed the jiren-addvalidatetag branch from ea8e93c to b7cae2d Compare July 5, 2017 22:07

amanohar suggested changes Jul 6, 2017

View reviewed changes

rjtsdl force-pushed the jiren-addvalidatetag branch from b7cae2d to 0275c3b Compare July 6, 2017 00:15

Jingtao Ren added 3 commits July 6, 2017 15:28

add validate tag for the purpose of required field

38fbe8d

import gopkg.in/go-playground/validator.v9

948bf61

apply gopkg.in/go-playground/validator.v9 for 2017-07-01 api model

2200585

rjtsdl force-pushed the jiren-addvalidatetag branch from 0275c3b to 2e4ceca Compare July 7, 2017 01:13

rjtsdl changed the title ~~add validate tag for the purpose of required field~~ [WIP]: add validate tag for the purpose of required field Jul 7, 2017

rjtsdl force-pushed the jiren-addvalidatetag branch from 2e4ceca to bd44bf9 Compare July 7, 2017 03:20

rjtsdl changed the title ~~[WIP]: add validate tag for the purpose of required field~~ add validate tag for the purpose of required field Jul 7, 2017

rjtsdl force-pushed the jiren-addvalidatetag branch 3 times, most recently from 0dfec88 to 2d114e7 Compare July 7, 2017 03:34

rjtsdl commented Jul 7, 2017

View reviewed changes

rjtsdl force-pushed the jiren-addvalidatetag branch 2 times, most recently from 37f6751 to 19d4760 Compare July 7, 2017 16:13

amanohar approved these changes Jul 7, 2017

View reviewed changes

add handler for validation errors

88291e3

rjtsdl force-pushed the jiren-addvalidatetag branch from 19d4760 to 88291e3 Compare July 7, 2017 18:42

rjtsdl merged commit 8a47cbd into Azure:master Jul 7, 2017

rjtsdl deleted the jiren-addvalidatetag branch July 7, 2017 20:40

add validate tag for the purpose of required field #886

add validate tag for the purpose of required field #886

Conversation

rjtsdl commented Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

rjtsdl commented Jun 28, 2017

JackQuincy commented Jun 28, 2017

weinong commented Jun 28, 2017

JiangtianLi commented Jun 28, 2017

rjtsdl commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanohar Jun 29, 2017 • edited Loading

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

rjtsdl commented Jul 5, 2017

JiangtianLi commented Jul 5, 2017

rjtsdl commented Jul 5, 2017

rjtsdl commented Jul 5, 2017 • edited Loading

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

rjtsdl Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

weinong Jul 6, 2017 • edited Loading

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

seanknox commented Jul 5, 2017

Choose a reason for hiding this comment

rjtsdl commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanohar commented Jul 7, 2017

rjtsdl commented Jul 7, 2017

weinong commented Jul 7, 2017

rjtsdl commented Jul 7, 2017

weinong commented Jul 7, 2017

rjtsdl commented Jul 7, 2017

rjtsdl commented Jul 7, 2017

rjtsdl commented Jun 28, 2017 •

edited

Loading

amanohar Jun 29, 2017 •

edited

Loading

rjtsdl commented Jul 5, 2017 •

edited

Loading

rjtsdl Jul 5, 2017 •

edited

Loading

weinong Jul 6, 2017 •

edited

Loading