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.

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

feat: Add script for yaml validation with kubeval #55

Merged
merged 1 commit into from
Nov 11, 2021

Conversation

mimotej
Copy link
Member

@mimotej mimotej commented Nov 9, 2021

Signed-off-by: Michal Drla mdrla@redhat.com

Related Issues and Dependencies

This introduces a breaking change

  • Yes
  • No

This Pull Request implements

This PR adds script which can be used in CI to validate YAML manifests using kubeval.
part of: https://github.com/operate-first/SRE/issues/107

Description

@sesheta sesheta added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2021
@sesheta sesheta requested a review from larsks November 9, 2021 12:29
@sesheta sesheta added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 9, 2021
@larsks
Copy link

larsks commented Nov 10, 2021

I was looking at https://github.com/mimotej/schema-store/tree/add-json-schemas-crds; you'll want to generate the --strict schemas as well -- without those, you won't be able to catch extraneous attributes and mis-spellings, because the non-strict schemas permit arbitrary attributes.

@mimotej
Copy link
Member Author

mimotej commented Nov 10, 2021

I was looking at https://github.com/mimotej/schema-store/tree/add-json-schemas-crds; you'll want to generate the --strict schemas as well -- without those, you won't be able to catch extraneous attributes and mis-spellings, because the non-strict schemas permit arbitrary attributes.

@larsks Good idea didn't think about that 👍. The question that only comes to my mind about this is: Should we keep both non-strict and strict schemas in this repository? Because I feel we wouldn't have currently any use for weaker schemas which let slip more error through the checks...

I have updated both script and schema-store repository PR to contain only strict variant of schemas.

@larsks
Copy link

larsks commented Nov 10, 2021

The question that only comes to my mind about this is: Should we keep both non-strict and strict schemas in this repository?

I don't see much use for the relaxed schemas in this context, and we can always add them in the future if we decide we have a use case that requires them. Having the strict schemas only right now seems fine.

@mimotej mimotej changed the title wip: feat: Add script for yaml validation with kubeval feat: Add script for yaml validation with kubeval Nov 10, 2021
@sesheta sesheta removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 10, 2021
@mimotej
Copy link
Member Author

mimotej commented Nov 10, 2021

/hold
Let's wait till operate-first/schema-store#1 will get merged

@sesheta sesheta added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 10, 2021
@HumairAK
Copy link
Member

HumairAK commented Nov 10, 2021

As a fan of having to wait less time for ci .... any particular reason why we don't just make this part of the kustomize build ci check? the script is nearly identical, you can just just run both checks and if either yields and error the script fails and errexits.

@mimotej
Copy link
Member Author

mimotej commented Nov 11, 2021

As a fan of having to wait less time for ci .... any particular reason why we don't just make this part of the kustomize build ci check? the script is nearly identical, you can just just run both checks and if either yields and error the script fails and errexits.

@HumairAK I don't feel like it is gonna take much longer to run it. kustomize build is prerequisite for manifest validation, but I would like more to see when I create PR, if the kustomize build failed or manifest validation failed and I don't want to immediately go looking in the logs. Upside of two scripts is that kustomize build check will be seperate and you can immediately tell what part of CI failed.

Signed-off-by: Michal Drla <mdrla@redhat.com>
@mimotej
Copy link
Member Author

mimotej commented Nov 11, 2021

/Unhold
Schemas were merged, so I have changed path to operate-first repository and it can be now merged.

@sesheta sesheta removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 11, 2021
Copy link
Member

@HumairAK HumairAK left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@sesheta sesheta added the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2021
@sesheta
Copy link
Member

sesheta commented Nov 11, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HumairAK, larsks

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sesheta sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 11, 2021
@sesheta sesheta merged commit bc8685b into operate-first:master Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants