You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/12/29 00:42:16 UTC
[GitHub] [airflow] jedcunningham opened a new pull request #20544: Use local definitions for k8s schema validation
jedcunningham opened a new pull request #20544:
URL: https://github.com/apache/airflow/pull/20544
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #20544: Use local definitions for k8s schema validation
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #20544:
URL: https://github.com/apache/airflow/pull/20544#discussion_r776159009
##########
File path: scripts/ci/pre_commit/pre_commit_vendor_k8s_json_schema.py
##########
@@ -0,0 +1,87 @@
+#!/usr/bin/env python3
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import json
+
+import requests
+
+K8S_DEFINITIONS = (
+ "https://raw.githubusercontent.com/yannh/kubernetes-json-schema"
+ "/master/v1.22.0-standalone-strict/_definitions.json"
+)
+VALUES_SCHEMA_FILE = "chart/values.schema.json"
+
+
+with open(VALUES_SCHEMA_FILE) as f:
+ schema = json.load(f)
+
+refs = set()
+
+
+def find_refs(props: dict):
+ for value in props.values():
+ if "$ref" in value:
+ refs.add(value["$ref"])
+
+ if "items" in value:
+ if "$ref" in value["items"]:
+ refs.add(value["items"]["$ref"])
+
+ if "properties" in value:
+ find_refs(value["properties"])
Review comment:
```suggestion
def find_refs(props: dict):
for value in props.values():
if "$ref" in value:
yield value["$ref"]
if "items" in value:
if "$ref" in value["items"]:
yield value["items"]["$ref"]
if "properties" in value:
yield from find_refs(value["properties"])
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #20544: Use local definitions for k8s schema validation
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #20544:
URL: https://github.com/apache/airflow/pull/20544#issuecomment-1002414889
> 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 👍.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #20544: Use local definitions for k8s schema validation
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20544:
URL: https://github.com/apache/airflow/pull/20544#issuecomment-1002743775
> 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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #20544: Use local definitions for k8s schema validation
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #20544:
URL: https://github.com/apache/airflow/pull/20544#issuecomment-1002340933
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #20544: Use local definitions for k8s schema validation
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20544:
URL: https://github.com/apache/airflow/pull/20544#issuecomment-1002342117
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham merged pull request #20544: Use local definitions for k8s schema validation
Posted by GitBox <gi...@apache.org>.
jedcunningham merged pull request #20544:
URL: https://github.com/apache/airflow/pull/20544
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #20544: Use local definitions for k8s schema validation
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #20544:
URL: https://github.com/apache/airflow/pull/20544#discussion_r776160033
##########
File path: scripts/ci/pre_commit/pre_commit_vendor_k8s_json_schema.py
##########
@@ -0,0 +1,87 @@
+#!/usr/bin/env python3
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import json
+
+import requests
+
+K8S_DEFINITIONS = (
+ "https://raw.githubusercontent.com/yannh/kubernetes-json-schema"
+ "/master/v1.22.0-standalone-strict/_definitions.json"
+)
+VALUES_SCHEMA_FILE = "chart/values.schema.json"
+
+
+with open(VALUES_SCHEMA_FILE) as f:
+ schema = json.load(f)
+
+refs = set()
+
+
+def find_refs(props: dict):
+ for value in props.values():
+ if "$ref" in value:
+ refs.add(value["$ref"])
+
+ if "items" in value:
+ if "$ref" in value["items"]:
+ refs.add(value["items"]["$ref"])
+
+ if "properties" in value:
+ find_refs(value["properties"])
+
+
+def get_remote_schema(url: str) -> dict:
+ req = requests.get(url)
+ req.raise_for_status()
+ return json.loads(req.text)
Review comment:
```suggestion
return req.json()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #20544: Use local definitions for k8s schema validation
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20544:
URL: https://github.com/apache/airflow/pull/20544#issuecomment-1002345416
> 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #20544: Use local definitions for k8s schema validation
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20544:
URL: https://github.com/apache/airflow/pull/20544#issuecomment-1002344349
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #20544: Use local definitions for k8s schema validation
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #20544:
URL: https://github.com/apache/airflow/pull/20544#discussion_r776159082
##########
File path: scripts/ci/pre_commit/pre_commit_vendor_k8s_json_schema.py
##########
@@ -0,0 +1,87 @@
+#!/usr/bin/env python3
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import json
+
+import requests
+
+K8S_DEFINITIONS = (
+ "https://raw.githubusercontent.com/yannh/kubernetes-json-schema"
+ "/master/v1.22.0-standalone-strict/_definitions.json"
+)
+VALUES_SCHEMA_FILE = "chart/values.schema.json"
+
+
+with open(VALUES_SCHEMA_FILE) as f:
+ schema = json.load(f)
+
+refs = set()
+
+
+def find_refs(props: dict):
+ for value in props.values():
+ if "$ref" in value:
+ refs.add(value["$ref"])
+
+ if "items" in value:
+ if "$ref" in value["items"]:
+ refs.add(value["items"]["$ref"])
+
+ if "properties" in value:
+ find_refs(value["properties"])
+
+
+def get_remote_schema(url: str) -> dict:
+ req = requests.get(url)
+ req.raise_for_status()
+ return json.loads(req.text)
+
+
+# Create 'definitions' if it doesn't exist or reset it
+schema["definitions"] = {}
+
+# Get the k8s defs
+defs = get_remote_schema(K8S_DEFINITIONS)
+
+# first find refs in our schema
+find_refs(schema["properties"])
Review comment:
```suggestion
refs = list(find_refs(schema["properties"]))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #20544: Use local definitions for k8s schema validation
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #20544:
URL: https://github.com/apache/airflow/pull/20544#discussion_r776159423
##########
File path: scripts/ci/pre_commit/pre_commit_vendor_k8s_json_schema.py
##########
@@ -0,0 +1,87 @@
+#!/usr/bin/env python3
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import json
+
+import requests
+
+K8S_DEFINITIONS = (
+ "https://raw.githubusercontent.com/yannh/kubernetes-json-schema"
+ "/master/v1.22.0-standalone-strict/_definitions.json"
+)
+VALUES_SCHEMA_FILE = "chart/values.schema.json"
+
+
+with open(VALUES_SCHEMA_FILE) as f:
+ schema = json.load(f)
+
+refs = set()
+
+
+def find_refs(props: dict):
+ for value in props.values():
+ if "$ref" in value:
+ refs.add(value["$ref"])
+
+ if "items" in value:
+ if "$ref" in value["items"]:
+ refs.add(value["items"]["$ref"])
+
+ if "properties" in value:
+ find_refs(value["properties"])
Review comment:
The code is just as long, but a tad more readable because the `refs` variable is created with a certain value and is not modified from another function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org