-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
Strict schema for k8s objects for values.yaml #19181
Conversation
We are now only missing a few types to have full and strict coverage.
After:
|
CC: @simplylizz |
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.
Still looks like WIP to me, keep it up
@mrbaguvix I have currently solved all the issues with the structure of objects. Now only the issues with the structures of the array remain, but I think we can solve that as a separate contribution. It is important to me that most of the problems are resolved, so following contributions that follow the conventions in this file will also declare the complete structure of the objects. Previously, even when someone wanted to use a reference, they weren't sure if it was a good idea. Here is example: #19175 (comment) |
I have resolved all the problems except the following
1 and 2: I don't know the correct structure. This field is also not covered by tests. |
5e684a3
to
68c6687
Compare
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.
Overall LGTM.
I'm wondering if we should hold this until after we cut 1.3.0
and have our next release be 2.0.0
. I could easily see folks having extra config in their values and this stricter schema being surprising. Thoughts?
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease. |
As for fields that have a structure defined with an external reference, users cannot define additional fields as this will cause an invalid k8s object to be generated. Here is an example: #19379 The problem is only with the objects that are defined in this schema, but we can remove them from this contribution and change them in the future. |
ed4be14
to
6c95d20
Compare
I deleted all added |
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. There are some helm unit tests failures though.
I looked at the CI problem and I think the OpenAPI specification has a bug. ResourceQuotaSpec has the following definition: {
"properties": {
"hard": {
"additionalProperties": {
"$ref": "https://raw.githubusercontent.com/yannh/kubernetes-json-schema/master/v1.21.0/_definitions.json#/definitions/io.k8s.apimachinery.pkg.api.resource.Quantity"
},
"type": [
"object",
"null"
]
},
... and {
"oneOf": [
{
"type": "string"
},
{
"type": "number"
}
],
"$schema": "http://json-schema.org/schema#",
"type": "object"
} The fields |
f1ebac5
to
af52ea3
Compare
Here issue: yannh/kubernetes-json-schema#11 |
Some fields in values.yml accept value that complies with the Kubernetes API, so we can verify that too.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.