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

Use local definitions for k8s schema validation #20544

Merged
merged 3 commits into from
Dec 29, 2021

Conversation

jedcunningham
Copy link
Member

Instead of using remote schemas to validate helm chart values, we will
vendor in the definitions locally so the chart can be used without
using outbound connections.

There is a precommit hook that will ensure only the definitions we use
are included in our schema file, as there are quite a few of them all
together otherwise.

closes: #20539

Instead of using remote schemas to validate helm chart values, we will
vendor in the definitions locally so the chart can be used without
using outbound connections.

There is a precommit hook that will ensure only the definitions we use
are included in our schema file, as there are quite a few of them all
together otherwise.
@jedcunningham
Copy link
Member Author

I'm not sure if it is worth the effort to only include the upstream defs we actually use, but it does cut it down from 17.5k lines to 2.6k, so figured it was worth exploring at least. Thoughts?

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Smart!

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Dec 29, 2021
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk
Copy link
Member

potiuk commented Dec 29, 2021

I'm not sure if it is worth the effort to only include the upstream defs we actually use, but it does cut it down from 17.5k lines to 2.6k, so figured it was worth exploring at least. Thoughts?

Makes perfect sense - especially that it's fully automated with pre-commit.

Eventually we might want to contribute (thought about it) support to add mapped loaders for refs and then we could even bundle all the definitions as separate files - https://github.com/xeipuuv/gojsonschema#loading-local-schemas - but that would have to be approved by helm maintainers and released.

@potiuk
Copy link
Member

potiuk commented Dec 29, 2021

I'm not sure if it is worth the effort to only include the upstream defs we actually use, but it does cut it down from 17.5k lines to 2.6k, so figured it was worth exploring at least. Thoughts?

The only problem with this is a way how to use some definitions that we do not use currently. I think it is worth to do it in the way that we can also remove the vendoing easily (reversing the process). The process would loook like this;

  1. if you want to work on the schema and add new stuff, run a script to un-vendor the refs.
  2. make your changes
  3. commit (and this will automatically vendor them in).

@jedcunningham
Copy link
Member Author

The only problem with this is a way how to use some definitions that we do not use currently. I think it is worth to do it in the way that we can also remove the vendoing easily (reversing the process). The process would loook like this;

Yeah, I think an easier way would be to simply remove all io.k8s.* definitions instead of resetting to {}. That would still get us the automatic k8s defs, while leaving any custom ones in place. I just pushed that change 👍.

@kaxil kaxil added this to the Airflow Helm Chart 1.4.0 milestone Dec 29, 2021
@jedcunningham jedcunningham marked this pull request as ready for review December 29, 2021 16:16
@jedcunningham jedcunningham requested a review from ashb as a code owner December 29, 2021 16:16
@potiuk
Copy link
Member

potiuk commented Dec 29, 2021

Yeah, I think an easier way would be to simply remove all io.k8s.* definitions instead of resetting to {}. That would still get us the automatic k8s defs, while leaving any custom ones in place. I just pushed that change 👍.

Cool!

@jedcunningham jedcunningham merged commit afd84f6 into apache:main Dec 29, 2021
@jedcunningham jedcunningham deleted the no_outbound_slim branch December 29, 2021 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools area:helm-chart Airflow Helm Chart full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Offline chart installs broken
4 participants