-
Notifications
You must be signed in to change notification settings - Fork 213
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
JSON schema checks #234
JSON schema checks #234
Conversation
…ris into rb/custom-checks
Codecov Report
@@ Coverage Diff @@
## master #234 +/- ##
=========================================
- Coverage 79.21% 71.31% -7.9%
=========================================
Files 12 13 +1
Lines 765 781 +16
=========================================
- Hits 606 557 -49
- Misses 134 186 +52
- Partials 25 38 +13
Continue to review full report at Codecov.
|
In the interest of keeping changes small, I'm going to cut this PR here. Future work:
|
Also note: while this PR is backward-compatible, this will likely get released as a major version update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks _Robert . Looks good I just had few questions.
cv.validateHealthChecks(conf, controllerName) | ||
|
||
err := applyContainerSchemaChecks(conf, controllerName, controllerType, &cv) | ||
// FIXME: don't panic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming this was left on purpose to be fixed in separate PR right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup - I've got another PR coming in after this one, and probably one more refactor after that.
} | ||
} | ||
} | ||
|
||
func (cv *ContainerValidation) validateSecurity(conf *config.Configuration, controllerName string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand why is only validate security is left here ...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The capabilities
check is harder to implement in JSON schema, so I'm leaving it here for now. It'll be gone in the next PR.
This is step 1 toward supporting custom checks written in JSON schema.
I think rewriting many of our current checks using JSON schema will help clean up the codebase a bunch too.