feat: validator for cobra cmds #82

ankithans · 2021-06-24T08:30:48Z

Methods

Validates the command and it's descendants

func Validate(cmd *cobra.Command) error // handles default errors declared in validator package
func ValidateCustom(cmd *cobra.Command, handleErrors func() error) error // on the top of default errors, user can define custom errors

To decide

what aspects we want to control
- For now : Use, Short, Long, Example, Args
How do we like this validator package to be served
- from factory?
- seperate from sdk?

@aerogear/charmil-core
closes #58

wtrocki · 2021-06-24T09:00:20Z

How do we like this validator package to be served
from factory?
seperate from sdk?

I think I have provided info for that in issues and chat. Basically the best would be for this to be completely separate library.
And as mentioned this path doesn't have very clear way to run this without investigation.
My take is that could be helper golang unit tests library and rules should be actual unit tests, but please research this properly.

wtrocki · 2021-06-24T09:17:20Z

I would really love us to come with rules that can declare the presence of the field. Length of the field etc.
This way we can build some generic tool that cover cobra structures and came up with more advanced rules.

I would love to see this done this week so we can integrate that with the RHOAS CLI next week as unit test or whatever you figure out.

wtrocki · 2021-06-24T09:54:26Z

examples/plugins/date/main.go

@@ -49,5 +53,11 @@ func DateCommand() (*cobra.Command, error) {
 		},
 	}

+	// default validation provided by validator package
+	validationErr := validator.Validate(cmd)


Yes.. while this is ok. It makes no sense.

Validation is development feature but we are using it at runtime. This needs to change.

wtrocki · 2021-06-24T09:56:29Z

validator/std_validator.go

+	return nil
+}
+
+// ValidateCustom will allow user to


This is wrong. Every rule should be custom rule. Create interface for rule with single method that validates single command and then traverse thru nested structure. This is easier than current architecture and will allow anyone to drop any rules - users will always need to define rules somehow or override them, but it should not require to actually write them - for example fieldExist or string length rule needs to have some arguments to be defined.

wtrocki · 2021-06-24T09:58:14Z

validator/std_validator.go

+func handleDefaultErrors(cmd *cobra.Command) error {
+	cmdPath := cmd.CommandPath()
+
+	if len(cmd.Use) < 1 {


Makes no sense in number of dimensions:

we are missing exact location that can be used to pinpoint file that causing issues

we are returning only single issue (rather than numerous issues during linting)

Hardcoded values.

wtrocki · 2021-06-24T18:52:12Z

please rebase conflicts. I have done some investigations and I feel like unit testing framework with assertions could work, but number of rules we would be able to pull will be limited to structure checks.

I think that is fine - tried to do AST linter and while you can do the same - it is way harder to write rules

ankithans · 2021-06-24T19:00:35Z

@wtrocki conflicts resolved
so should I proceed with AST linter? Hit and trial is good approach for doing AST?

ankithans · 2021-06-24T19:08:59Z

how will the user add the custom rules. Will he be writing rules with AST?
or the user will be only using the rules provided by us, selecting what they want to apply?

wtrocki · 2021-06-24T20:22:48Z

No AST. Lets finalize this PR properly using approach we agred on. We need to lower expectations to adjust to progress. AST linter is 3-4 times more work and our use cases are simple enough to use it without ast/static code analisis

wtrocki · 2021-06-24T20:30:55Z

how will the user add the custom rules. Will he be writing rules with AST?

create golang interface representing rule and build number of rules based on them. Users could add their own. No AST

wtrocki · 2021-06-25T09:37:26Z

@ankithans I think it will be good to have class and sequence diagram (can be on piece of paper) to visualize what we want to do.

wtrocki · 2021-06-25T09:42:57Z

validator/length.go

+	cmdPath := cmd.CommandPath()
+
+	use := cmd.Use
+	useErr := validateField(l.Use, use, cmdPath)


This part might bring some maintenance challenges and breaks DRY rule.

We can use reflect package to get values by strings there is no need to repeat them

wtrocki · 2021-06-25T09:54:29Z

validator/length.go

+)
+
+type LengthRule struct {
+	Use     Limit


While this is clever I think it restricts us how rules are build.
My take is that rule:

Has name

Has Function that does something - Doesn't have any fields

Has specific arguments that configure how rule works.

In this case name is defined as Struct but I think we also need name as constant.

this way we can build LengthRule with dictionary of key values, where key could be cobra structure key like Use and values might be functions, min, max etc.

LengthRule(Arguments{ "Use": Limit{3,10}, "Short": Limit{3,10}, })

edit : ignore this

type LengthRuleMap map[string]Limit // instead of struct

// user var r1 validator.Rule = &validator.LengthRuleMap{ "Use": validator.Limit{Min: 1, Max: 5}, "Short": validator.Limit{Min: 4, Max: 5}, "Long": validator.Limit{Min: 5, Max: 20}, "Example": validator.Limit{Min: 5, Max: 30}, }

yes this helps in traversing and don't break DRY
here we will need to validate the key in dict, to map with cobra command struct

wtrocki · 2021-06-25T09:56:16Z

examples/plugins/date/main.go

-	validationErr := validator.Validate(cmd)
-	if validationErr != nil {
-		return nil, validationErr
+	var r validator.Rule = &validator.LengthRule{


This approach as number of drawbacks:

it is fixed to the point where we need to redeclare every field

cannot be declarative

Doesn't provide us base for adding other rules (missing interface for rule)

wtrocki · 2021-06-25T09:57:02Z

Amazing work! This is getting closer - but still needs to be refined and we need more rules. Nice to see some non trivial code happening

wtrocki · 2021-06-25T10:51:38Z

Rough example showing what I meant by using reflection:
https://play.golang.org/p/Y9tSo1k8FsS

ankithans · 2021-06-25T12:35:54Z

@aerogear/charmil-core
we have length & mustPresent validator for now(yet to add docs). Any more ideas for what would we like to validate in cobra command?

Rough example showing what I meant by using reflection:
https://play.golang.org/p/Y9tSo1k8FsS

Thanks for this!! reflect is great 🙌

wtrocki · 2021-06-25T13:12:28Z

lets start with those two rules for now

wtrocki · 2021-06-25T13:15:40Z

validator/length.go

+	err := validateLength(cmd, l)
+	errors = append(errors, err...)
+
+	for _, child := range cmd.Commands() {


we are doing only one level of commands. we should check all commands including children of children

wtrocki · 2021-06-25T13:16:50Z

validator/length.go

+	var errors []error
+
+	for fieldName, limits := range l.Limits {
+		reflectValue := reflect.ValueOf(cmd).Elem().FieldByName(fieldName).String()


no error handling/input validation

wtrocki · 2021-06-25T13:17:16Z

validator/length.go

+	}
+
+	if length < limit.Min {
+		return fmt.Errorf("%s in %s: length should be atleast %d", value, path, limit.Min)


typo atleast

wtrocki · 2021-06-25T13:18:47Z

validator/length.go

+		return true, fmt.Errorf("max and min must be greater than 0")
+	}
+	if limit.Max == 0 && limit.Min == 0 {
+		return false, fmt.Errorf("limit not set")


lack of good message patterns.

Limit to what is not set? Where?

Would use rule name for all rule specific errors.

wtrocki · 2021-06-25T13:19:48Z

validator/length.go

+}
+
+func validateField(limit Limit, value string, path string) error {
+	length := len(value)


Lets add verbose mode that prints debug info for each rule cmd field

rebase with main

wtrocki · 2021-06-29T09:19:19Z

Rebase needed
This branch cannot be rebased due to conflicts

One comment missed

rebase

ankithans · 2021-06-29T09:28:07Z

done @wtrocki

wtrocki · 2021-06-29T09:35:07Z

Merged with 101 comments:
https://en.wikipedia.org/wiki/101_(topic)

ankithans · 2021-06-29T09:36:46Z

"one-oh-one" 😅🥲

wtrocki · 2021-06-29T09:38:35Z

Btw.. This is pretty much still work in progress change in main. Let's work on refining it better.

ankithans · 2021-06-29T09:44:12Z

yes sure, first let's do unit tests properly and find bugs by trying with big CLI's
first #88 #94
then #84 ,#85, #86

then we should do CLI? or in between what do you suggest?

wtrocki · 2021-06-29T09:52:07Z

Priority nr1 is to do rhoas integration.

jackdelahunt · 2021-07-03T00:09:21Z

@ankithans I just worked with the validator functionality for the first and it looks really nice just had a few questions on going forward on what we are looking to refine.

We check the rules config values to make sure they are not dumb, like is max less then min etc. here but I don't see why we can't check these fields when we create the rule config instead of every time we run the validator.
Also cases like if max is 0 then don't check should be documented but again you may already be planning this. Code

ankithans · 2021-07-03T03:03:43Z

We check the rules config values to make sure they are not dumb, like is max less then min etc. here but I don't see why we can't check these fields when we create the rule config instead of every time we run the validator.

I think, it's better to be part of rule, which can be run by ruleExecutor. As every rule will have some checking like this. So if we do that in ruleExecutor, things might get messed up. WDYS?

Also cases like if max is 0 then don't check should be documented but again you may already be planning this. Code

Yep thanks for suggesting, need to document again

jackdelahunt · 2021-07-03T05:16:48Z

if we do that in ruleExecutor, things might get messed up.

Yes that being in the validator would be annoying but I don't think it has to be.

I am gonna make a quick pr today on what think we can do as I think I have a good idea. We also have to be mindful of yaml/json configs for the rules. But I will get back to you.

ankithans added the enhancement New feature or request label Jun 24, 2021

ankithans requested review from wtrocki and namit-chandwani June 24, 2021 08:30

ankithans self-assigned this Jun 24, 2021

wtrocki reviewed Jun 24, 2021

View reviewed changes

wtrocki self-requested a review June 24, 2021 18:42

ankithans force-pushed the cobra-validator branch from 78e7f12 to e549448 Compare June 25, 2021 09:35

wtrocki reviewed Jun 25, 2021

View reviewed changes

ankithans requested a review from wtrocki June 25, 2021 12:40

wtrocki reviewed Jun 25, 2021

View reviewed changes

feat: validator for cobra cmds

41f4696

rebase with main

ankithans added 16 commits June 29, 2021 14:49

format using gofmt

c1e8150

rebase

feat: length validator

949eb7e

feat: use reflect & map for length rule

04c97b1

feat: add mustpresent validator

cee85e8

feat: error structure in validators

d76e98b

feat: add verbose mode for rules

bdff923

feat: status in verbose mode

f11645b

fix: traverse all levels in commands

1a3be98

fix: validate during reflecting

078661f

docs: dev docs for validator

20cf9dc

chore: refactor rules & traversing of cmds

f751681

chore: refactor validator

aae5c35

feat: user facing documentation

a78b2f4

refactor: all rules in executor

4990b44

feat: merge default and user provided rulesconfig

4f8a4c8

address review comments

08d3ccc

ankithans force-pushed the cobra-validator branch from 4373863 to 08d3ccc Compare June 29, 2021 09:24

wtrocki merged commit cd285f2 into main Jun 29, 2021

wtrocki deleted the cobra-validator branch June 29, 2021 09:35

feat: validator for cobra cmds #82

feat: validator for cobra cmds #82

Conversation

ankithans commented Jun 24, 2021

Methods

To decide

wtrocki commented Jun 24, 2021

wtrocki commented Jun 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki commented Jun 24, 2021

ankithans commented Jun 24, 2021

ankithans commented Jun 24, 2021

wtrocki commented Jun 24, 2021 • edited

wtrocki commented Jun 24, 2021 • edited

wtrocki commented Jun 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankithans Jun 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki commented Jun 25, 2021

wtrocki commented Jun 25, 2021

ankithans commented Jun 25, 2021 • edited

wtrocki commented Jun 25, 2021 • edited

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

wtrocki commented Jun 29, 2021

ankithans commented Jun 29, 2021

wtrocki commented Jun 29, 2021

ankithans commented Jun 29, 2021

wtrocki commented Jun 29, 2021

ankithans commented Jun 29, 2021

wtrocki commented Jun 29, 2021

jackdelahunt commented Jul 3, 2021

ankithans commented Jul 3, 2021

jackdelahunt commented Jul 3, 2021

wtrocki commented Jun 24, 2021 •

edited

wtrocki commented Jun 24, 2021 •

edited

ankithans Jun 25, 2021 •

edited

ankithans commented Jun 25, 2021 •

edited

wtrocki commented Jun 25, 2021 •

edited