Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create validator object to give more validation flexibility #2484

Merged
merged 12 commits into from
Aug 2, 2023

Conversation

MustafaSaber
Copy link
Member

@MustafaSaber MustafaSaber commented Jul 26, 2023

This will help with #2478 to try improving validation and how to get around some DataClients tests

Also this run eskip.ParseFilters by default so that's a step forward.

Related to #1618

@MustafaSaber MustafaSaber force-pushed the rg-validator-object branch from 5296ff6 to 8ea1cee Compare July 26, 2023 15:29
return rl, err
}

type RouteGroupValidator struct {
Copy link
Member Author

@MustafaSaber MustafaSaber Jul 26, 2023

Choose a reason for hiding this comment

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

Thought about making Validator interface, something like

type Validator interface {
          Validate(item interface{}) error
}

but I think it's to early for this. It was to make all these validate functions to be used in other places and interface{} wasn't the best thing to use in this place in my opinion.

Copy link
Member

Choose a reason for hiding this comment

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

You can also use an interface function that doesn't pass a value as func argument.
Then you can use proper OO to figure out how to implement the Validate() error interface for a given type.

@MustafaSaber MustafaSaber force-pushed the rg-validator-object branch from 8ea1cee to 5b915c7 Compare July 26, 2023 15:36
@MustafaSaber
Copy link
Member Author

Real change is from L263 to EOF rest are just moved from routegroup file

@AlexanderYastrebov AlexanderYastrebov changed the title Create validator object to give more validation flexiability Create validator object to give more validation flexibility Jul 27, 2023
@MustafaSaber MustafaSaber force-pushed the rg-validator-object branch from 4a838db to c36ad46 Compare July 27, 2023 12:14
@MustafaSaber MustafaSaber force-pushed the rg-validator-object branch from 0f98572 to 4c48a48 Compare July 27, 2023 14:58
@MustafaSaber MustafaSaber self-assigned this Jul 27, 2023
@MustafaSaber
Copy link
Member Author

Still need to add some extra test cases to parsing filters improvement

@MustafaSaber MustafaSaber force-pushed the rg-validator-object branch 2 times, most recently from ed092f3 to 20fce69 Compare August 1, 2023 10:53
Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>
Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>
Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>
Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>
Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>
Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>
Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>
…eskip in case of all valid grammer.

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>
Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>
…ming

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>
Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>
var err []error
for _, r := range item.Spec.Routes {
for _, p := range r.Predicates {
_, e := eskip.ParsePredicates(p)
Copy link
Member

Choose a reason for hiding this comment

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

Here we actually allow a list of predicates and I think dataclient does the same while parsing.
This is an undocumented feature that might be surprising for users.
Lets address this in a separate PR after we check existing cluster resources.

Copy link
Member

Choose a reason for hiding this comment

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

Same for the filters.

Signed-off-by: Mustafa Abdelrahman <mustafa.abdelrahman@zalando.de>
@AlexanderYastrebov
Copy link
Member

👍

1 similar comment
@MustafaSaber
Copy link
Member Author

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants