-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add support for preflight check configuration #900
Conversation
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.
Looks good to me apart from Bryce's comment.
Should we add/update the preflight permissions e2e tests with a config example? (I am okay with just the unit tests as well)
Would would also have to add the preflight option in the configuration docs.
I'll create a new PR for the docs. The unit tests cover the code as written. IMHO, an e2e would be more appropriate once a preflight check adds configuration settings; otherwise, the e2e would effectively be a duplicate test. |
This adds configuration file support (`preflightRules`) for preflight checks. Each preflight check must add a `SetConfig()` method to accept configuration from the Preflight registry. Signed-off-by: Todd Short <todd.short@me.com>
Documentation PR: carvel-dev/carvel#740 |
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.
LGTM! Thank you so much for the contribution ❤️
What this PR does / why we need it:
This adds configuration file support (
preflightRules
) for preflight checks. Each preflight check must add aSetConfig()
method to accept configuration from the Preflight registry.Which issue(s) this PR fixes:
Fixes #899
Does this PR introduce a user-facing change?
Additional Notes for your reviewer:
Review Checklist:
a link to that PR
change
Additional documentation e.g., Proposal, usage docs, etc.:
Proposal carvel-dev/carvel#729