Skip to content

kev-409 Extend json validation for probes#412

Merged
mangas merged 3 commits intomasterfrom
kev-409-json-validation-probes
Mar 22, 2021
Merged

kev-409 Extend json validation for probes#412
mangas merged 3 commits intomasterfrom
kev-409-json-validation-probes

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
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.

Copy link
Member

@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?

@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
Copy link
Member

@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

@ezodude ezodude self-requested a review March 19, 2021 14:44
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...

@mangas mangas force-pushed the kev-409-json-validation-probes branch from 34602fa to d7ee773 Compare March 22, 2021 11:22
return func(re gojsonschema.ResultError) bool {
for _, t := range ts {
if t == re.Type() {
return true
Copy link
Member

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants