-
Notifications
You must be signed in to change notification settings - Fork 229
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
issue #47 : introduces --skip-crd-schema-miss flag #127
Conversation
Nice one! |
@garethr can you take a look? |
Friendly bump on this PR - we've just run into exactly this |
Pretty please merge this! |
Thanks, I think allowing for skipping missing schemas is a good first step towards better support for CRDs. Thanks a lot for taking a first run at this. Unfortunately I think the flag is misleading here. It flag won't just affect CRDs, but any resource for which the schema isn't loaded. So in the case of say a dropped network connection preventing the loading of a schema this would also allow it to pass. I think it's also probably worth issuing a warning about the schemas not being validated. Let me know if you would like to update this PR, otherwise I'm happy to finish off. |
@garethr thanks for the feedback! I have several questions, would appreciate whenever you have time you can give some answers to:
|
@emirozer I'm pretty easy on the implementation, so whatever you think is best to the first question. I tend to obsess over the user interface a bit more, as I'd prefer to keep that stable and clear, while the implementation can always evolve under the hood. I'm fine with step one being a flag which ignores network failures too. Renaming what you have to Having an explicit list of resources to skip validation would be good as well, but could come later if preferred. I'd suggest changing the name here as well, even if the main usecase might be for CRDs it would work with any resource types. Maybe Thanks for taking a look, a number of folks have mentioned this. |
You are definitely much better at naming flags than I am that's for sure haha :D EDIT: ok so that's done - I will see if I can find a smart/clean way of handling network failures |
It seems like it's ready to merge. @garethr can you take a look please? |
Adding another bump - this remains an important workaround that's much needed. |
…kip schema validation
This seems to be the way to be sure that the candidate to be evaluated is a CRD not a normal resource
The further change to determine potential network issues, if I'm not mistaken, seem to be has to handled in gojsonschema dependency. I tried to do a work around but it was quite ugly so i didn't push it. I rebased on master and fixed conflicts. I think we should just merge this maybe.. |
@emirozer Looks like there has been a regression for us with the latest changes 3 days ago... So given these manifests:
With this kubeval patch prior to 3 days ago we were getting the following correct output:
Now with the latest changes in the tip of this PR we are getting the following output, we should be seeing them as successfully skipped:
|
Yep @mikespokefire , thx for the heads up, regression caused by recent changes to master. I will push an update now |
@emirozer thanks for that fix, can confirm it fixed that issue, but now we have a slightly different issue as a result of that change. We are now getting some false positives when there should be errors, take the following manifest:
When running
However, when running
|
I think your unit tests for this change are also significantly affected by #141 |
Apologies for taking a while to get to merging this in, lots of travel and new job, etc. I've just added docs and acceptance tests as well and merged to master. I'll get a new version of kubeval released as well with the change. Thanks to @emirozer for the PR and everyone else for the reviews. |
@garethr Thanks for merging this. We are still seeing #127 (comment) as an issue even with the 0.11.0 release. Would you prefer I raise it as a separate issue rather than trying to track it in here? |
@mikespokefire Your issue is fixed in #150
|
Hi,
I made a quick fix for this issue, let me know if you are okay with this approach or not :)
Thx for your time!