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.

Sign up for GitHub

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

Jump to bottom

kev-409 Extend json validation for probes #412

Merged
merged 3 commits into from
Mar 22, 2021
Merged

kev-409 Extend json validation for probes #412

merged 3 commits into from
Mar 22, 2021

Conversation

mangas
Copy link
Contributor

@mangas mangas commented Mar 18, 2021

No description provided.

@mangas mangas force-pushed the kev-409-json-validation-probes branch from 4b29aa3 to c2cc411 Compare March 18, 2021 16:12
@mangas mangas marked this pull request as ready for review March 18, 2021 16:33
@mangas mangas requested review from ezodude and marcinc March 18, 2021 16:40
ezodude
ezodude reviewed Mar 18, 2021
View reviewed changes
Copy link
Contributor

@ezodude ezodude left a comment

Choose a reason for hiding this comment

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

Would appreciate feedback on comment.

pkg/kev/config/schema.go Show resolved Hide resolved
marcinc
marcinc reviewed Mar 18, 2021
View reviewed changes
Copy link
Contributor

@marcinc marcinc left a comment

Choose a reason for hiding this comment

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

Left a few comments. Should this PR also address the dependent config fields validations, based on selected probe type?

pkg/kev/config/schema.go Outdated Show resolved Hide resolved
pkg/kev/services.go Outdated Show resolved Hide resolved
pkg/kev/services_test.go Show resolved Hide resolved
@mangas mangas marked this pull request as draft March 19, 2021 12:12
@mangas mangas marked this pull request as ready for review March 19, 2021 12:36
marcinc
marcinc reviewed Mar 19, 2021
View reviewed changes
Copy link
Contributor

@marcinc marcinc left a comment

Choose a reason for hiding this comment

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

A few more little comments

pkg/kev/services.go Outdated Show resolved Hide resolved
pkg/kev/services.go Outdated Show resolved Hide resolved
pkg/kev/services_test.go Outdated Show resolved Hide resolved
pkg/kev/services_test.go Outdated Show resolved Hide resolved
pkg/kev/services_test.go Outdated Show resolved Hide resolved
@ezodude ezodude self-requested a review March 19, 2021 14:44
ezodude
ezodude reviewed Mar 19, 2021
View reviewed changes
Copy link
Contributor

@ezodude ezodude left a comment

Choose a reason for hiding this comment

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

All good - minor typos and such...

pkg/kev/services_test.go Outdated Show resolved Hide resolved
@mangas mangas force-pushed the kev-409-json-validation-probes branch from 34602fa to d7ee773 Compare March 22, 2021 11:22
marcinc
marcinc reviewed Mar 22, 2021
View reviewed changes
pkg/kev/services.go Outdated
return func(re gojsonschema.ResultError) bool {
for _, t := range ts {
if t == re.Type() {
return true
Copy link
Contributor

Choose a reason for hiding this comment

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

as you exclude I guess this should return false, and true by default otherwise?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @marcinc here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise, it makes sense. 👌

As a final nice to have, it would be great if we can move the schema logic into a separate file if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, this is why the build was failing, it's fixed now. Do you think this is more clear/easier to understand without a lot of context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also I agree with this not being the right place for this but is there a good place? Is it going to be re-used somewhere else? I wonder if this would make sense as a contribution to the upstream repo

@mangas mangas merged commit ded7b0f into master Mar 22, 2021
@mangas mangas deleted the kev-409-json-validation-probes branch March 22, 2021 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcinc marcinc marcinc left review comments

@ezodude ezodude ezodude approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mangas @marcinc @ezodude