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 2020/10/14 19:58:15 UTC
[GitHub] [airflow] dimberman opened a new pull request #11533: Create job for airflow migrations
dimberman opened a new pull request #11533:
URL: https://github.com/apache/airflow/pull/11533
Creating airflow migrations should run seperately from the user creation
job, as many users might not want to create users on deployment.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on a change in pull request #11533: Create job for airflow migrations
Posted by GitBox <gi...@apache.org>.
dimberman commented on a change in pull request #11533:
URL: https://github.com/apache/airflow/pull/11533#discussion_r504939885
##########
File path: chart/templates/create-user-job.yaml
##########
@@ -57,14 +57,6 @@ spec:
- name: {{ template "registry_secret" . }}
{{- end }}
containers:
- - name: run-airflow-migrations
Review comment:
@mik-laj I don't think it matters as this is a job, so it'll keep failing until it passes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #11533: Create job for airflow migrations
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11533:
URL: https://github.com/apache/airflow/pull/11533#discussion_r504998933
##########
File path: chart/templates/migrate-database-job.yaml
##########
@@ -0,0 +1,69 @@
+# 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.
+
+################################
+## Airflow Create User Job
+#################################
+{{- if .Values.webserver.defaultUser.enabled }}
Review comment:
This seems unexpected, but we would easily detect it if we had unit tests to check the resources that are created. For now, we have one that uses YAML files, but I have the impression that it is not very popular and has a lot of limitations. Alternatively, we can use any other framework. If we choose python and unit test it might look like this.
```pytthon
import subprocess
import unittest
from tempfile import NamedTemporaryFile
import yaml
from typing import Dict, Optional
OBJECT_COUNT_IN_BASIC_DEPLOYMENT = 22
class TestBaseChartTest(unittest.TestCase):
def render_chart(self, name, values: Optional[Dict] = None):
values = values or {}
with NamedTemporaryFile() as tmp_file:
content = yaml.dump(values)
tmp_file.write(content.encode())
print(content)
tmp_file.flush()
templates = subprocess.check_output(["helm", "template", name, "..", '--values', tmp_file.name])
k8s_objects = yaml.load_all(templates)
k8s_objects = [k8s_object for k8s_object in k8s_objects if k8s_object]
import pprint
pprint.pprint(k8s_objects)
return k8s_objects
def test_basic_deployments(self):
k8s_objects = self.render_chart("TEST-BASIC", {"chart": {'metadata': 'AA'}})
list_of_kind_names_tuples = [
(k8s_object['kind'], k8s_object['metadata']['name'])
for k8s_object in k8s_objects
]
self.assertEqual(
list_of_kind_names_tuples,
[
('ServiceAccount', 'TEST-BASIC-scheduler'),
('ServiceAccount', 'TEST-BASIC-webserver'),
('ServiceAccount', 'TEST-BASIC-worker'),
('Secret', 'TEST-BASIC-postgresql'),
('Secret', 'TEST-BASIC-airflow-metadata'),
('Secret', 'TEST-BASIC-airflow-result-backend'),
('ConfigMap', 'TEST-BASIC-airflow-config'),
('ClusterRole', 'TEST-BASIC-pod-launcher-role'),
('ClusterRoleBinding', 'TEST-BASIC-pod-launcher-rolebinding'),
('Service', 'TEST-BASIC-postgresql-headless'),
('Service', 'TEST-BASIC-postgresql'),
('Service', 'TEST-BASIC-statsd'),
('Service', 'TEST-BASIC-webserver'),
('Deployment', 'TEST-BASIC-scheduler'),
('Deployment', 'TEST-BASIC-statsd'),
('Deployment', 'TEST-BASIC-webserver'),
('StatefulSet', 'TEST-BASIC-postgresql'),
('Secret', 'TEST-BASIC-fernet-key'),
('Secret', 'TEST-BASIC-redis-password'),
('Secret', 'TEST-BASIC-broker-url'),
('Job', 'TEST-BASIC-create-user'),
('Job', 'TEST-BASIC-run-airflow-migrations')
]
)
self.assertEqual(OBJECT_COUNT_IN_BASIC_DEPLOYMENT, len(k8s_objects))
def test_basic_deployment_without_default_users(self):
k8s_objects = self.render_chart("TEST-BASIC", {"webserver": {'defaultUser': {'enabled': False}}})
list_of_kind_names_tuples = [
(k8s_object['kind'], k8s_object['metadata']['name'])
for k8s_object in k8s_objects
]
self.assertNotIn(('Job', 'TEST-BASIC-create-user'), list_of_kind_names_tuples)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #11533: Create job for airflow migrations
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11533:
URL: https://github.com/apache/airflow/pull/11533#discussion_r504998933
##########
File path: chart/templates/migrate-database-job.yaml
##########
@@ -0,0 +1,69 @@
+# 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.
+
+################################
+## Airflow Create User Job
+#################################
+{{- if .Values.webserver.defaultUser.enabled }}
Review comment:
This seems unexpected, but we would easily detect it if we had unit tests to check the resources that are created. For now, we have one that uses YAML files, but I have the impression that it is not very popular and has a lot of limitations. Alternatively, we can use any other framework. If we choose python and unit test it might look like this.
```python
import subprocess
import unittest
from tempfile import NamedTemporaryFile
import yaml
from typing import Dict, Optional
OBJECT_COUNT_IN_BASIC_DEPLOYMENT = 22
class TestBaseChartTest(unittest.TestCase):
def render_chart(self, name, values: Optional[Dict] = None):
values = values or {}
with NamedTemporaryFile() as tmp_file:
content = yaml.dump(values)
tmp_file.write(content.encode())
print(content)
tmp_file.flush()
templates = subprocess.check_output(["helm", "template", name, "..", '--values', tmp_file.name])
k8s_objects = yaml.load_all(templates)
k8s_objects = [k8s_object for k8s_object in k8s_objects if k8s_object]
import pprint
pprint.pprint(k8s_objects)
return k8s_objects
def test_basic_deployments(self):
k8s_objects = self.render_chart("TEST-BASIC", {"chart": {'metadata': 'AA'}})
list_of_kind_names_tuples = [
(k8s_object['kind'], k8s_object['metadata']['name'])
for k8s_object in k8s_objects
]
self.assertEqual(
list_of_kind_names_tuples,
[
('ServiceAccount', 'TEST-BASIC-scheduler'),
('ServiceAccount', 'TEST-BASIC-webserver'),
('ServiceAccount', 'TEST-BASIC-worker'),
('Secret', 'TEST-BASIC-postgresql'),
('Secret', 'TEST-BASIC-airflow-metadata'),
('Secret', 'TEST-BASIC-airflow-result-backend'),
('ConfigMap', 'TEST-BASIC-airflow-config'),
('ClusterRole', 'TEST-BASIC-pod-launcher-role'),
('ClusterRoleBinding', 'TEST-BASIC-pod-launcher-rolebinding'),
('Service', 'TEST-BASIC-postgresql-headless'),
('Service', 'TEST-BASIC-postgresql'),
('Service', 'TEST-BASIC-statsd'),
('Service', 'TEST-BASIC-webserver'),
('Deployment', 'TEST-BASIC-scheduler'),
('Deployment', 'TEST-BASIC-statsd'),
('Deployment', 'TEST-BASIC-webserver'),
('StatefulSet', 'TEST-BASIC-postgresql'),
('Secret', 'TEST-BASIC-fernet-key'),
('Secret', 'TEST-BASIC-redis-password'),
('Secret', 'TEST-BASIC-broker-url'),
('Job', 'TEST-BASIC-create-user'),
('Job', 'TEST-BASIC-run-airflow-migrations')
]
)
self.assertEqual(OBJECT_COUNT_IN_BASIC_DEPLOYMENT, len(k8s_objects))
def test_basic_deployment_without_default_users(self):
k8s_objects = self.render_chart("TEST-BASIC", {"webserver": {'defaultUser': {'enabled': False}}})
list_of_kind_names_tuples = [
(k8s_object['kind'], k8s_object['metadata']['name'])
for k8s_object in k8s_objects
]
self.assertNotIn(('Job', 'TEST-BASIC-create-user'), list_of_kind_names_tuples)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on a change in pull request #11533: Create job for airflow migrations
Posted by GitBox <gi...@apache.org>.
dimberman commented on a change in pull request #11533:
URL: https://github.com/apache/airflow/pull/11533#discussion_r504955240
##########
File path: chart/templates/create-user-job.yaml
##########
@@ -57,14 +57,6 @@ spec:
- name: {{ template "registry_secret" . }}
{{- end }}
containers:
- - name: run-airflow-migrations
Review comment:
@mik-laj this code is already tested. If the migrations don't happen, the k8s tests never launch. Also we're not adding any code, just moving code around. I'm glad to consider tests if you have an idea, but AFAIK this deployment IS tested as we have system tests.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #11533: Create job for airflow migrations
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11533:
URL: https://github.com/apache/airflow/pull/11533#discussion_r504965212
##########
File path: chart/templates/create-user-job.yaml
##########
@@ -57,14 +57,6 @@ spec:
- name: {{ template "registry_secret" . }}
{{- end }}
containers:
- - name: run-airflow-migrations
Review comment:
The code is under high-level testing, but that doesn't mean we shouldn't think about lower-level testing. I don't have much experience testing Helm Chart yet, but if you change the behavior of the code, the tests should also be updated.
I am able to imagine that our test could consist in generating the YAML code and checking only the type and name of the resource. That would be enough because we would know that these resources are really being created.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #11533: Create job for airflow migrations
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11533:
URL: https://github.com/apache/airflow/pull/11533#discussion_r504954795
##########
File path: chart/templates/create-user-job.yaml
##########
@@ -57,14 +57,6 @@ spec:
- name: {{ template "registry_secret" . }}
{{- end }}
containers:
- - name: run-airflow-migrations
Review comment:
I am still thinking about the tests. Should we add any tests? My heart hurts when it adds code that does not have automatic or semi-automatic tests. Is it possible to add some automated tests here so that future contributors can verify our assumptions?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman merged pull request #11533: Create job for airflow migrations
Posted by GitBox <gi...@apache.org>.
dimberman merged pull request #11533:
URL: https://github.com/apache/airflow/pull/11533
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on a change in pull request #11533: Create job for airflow migrations
Posted by GitBox <gi...@apache.org>.
dimberman commented on a change in pull request #11533:
URL: https://github.com/apache/airflow/pull/11533#discussion_r505084611
##########
File path: chart/templates/migrate-database-job.yaml
##########
@@ -0,0 +1,69 @@
+# 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.
+
+################################
+## Airflow Create User Job
+#################################
+{{- if .Values.webserver.defaultUser.enabled }}
Review comment:
@mik-laj I think your point is well taken and I think better testing would be a really good idea. Going forward I do intend to maintain the helm chart, and I agree that we need to enforce coverage going forward. I've added a basic test for this change, and I think it'll be good to enforce tests based on the change you're making.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #11533: Create job for airflow migrations
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11533:
URL: https://github.com/apache/airflow/pull/11533#discussion_r505001266
##########
File path: chart/templates/migrate-database-job.yaml
##########
@@ -0,0 +1,69 @@
+# 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.
+
+################################
+## Airflow Create User Job
+#################################
+{{- if .Values.webserver.defaultUser.enabled }}
Review comment:
I do not require this to be done in this change, but I am trying to help as I see the lack of a unit tests for Helm Chart is a commonly reported problem. I also do not maintain the Helm Chart, only sporadic contributions and uses, so I find it difficult to make these decisions myself.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #11533: Create job for airflow migrations
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11533:
URL: https://github.com/apache/airflow/pull/11533#discussion_r505096097
##########
File path: chart/templates/migrate-database-job.yaml
##########
@@ -0,0 +1,69 @@
+# 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.
+
+################################
+## Airflow Create User Job
Review comment:
@dimberman
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #11533: Create job for airflow migrations
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11533:
URL: https://github.com/apache/airflow/pull/11533#discussion_r504963944
##########
File path: chart/templates/migrate-database-job.yaml
##########
@@ -0,0 +1,69 @@
+# 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.
+
+################################
+## Airflow Create User Job
Review comment:
Can you update this title?
##########
File path: chart/templates/migrate-database-job.yaml
##########
@@ -0,0 +1,69 @@
+# 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.
+
+################################
+## Airflow Create User Job
+#################################
+{{- if .Values.webserver.defaultUser.enabled }}
Review comment:
Is it needed?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #11533: Create job for airflow migrations
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11533:
URL: https://github.com/apache/airflow/pull/11533#discussion_r504939294
##########
File path: chart/templates/create-user-job.yaml
##########
@@ -57,14 +57,6 @@ spec:
- name: {{ template "registry_secret" . }}
{{- end }}
containers:
- - name: run-airflow-migrations
Review comment:
Should we add wait-for-airflow-migrations here?
https://github.com/apache/airflow/blob/7b7cc3c391d756e0ef55df8f0c674da4de3fb4f0/chart/templates/webserver/webserver-deployment.yaml#L78
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman commented on a change in pull request #11533: Create job for airflow migrations
Posted by GitBox <gi...@apache.org>.
dimberman commented on a change in pull request #11533:
URL: https://github.com/apache/airflow/pull/11533#discussion_r504955240
##########
File path: chart/templates/create-user-job.yaml
##########
@@ -57,14 +57,6 @@ spec:
- name: {{ template "registry_secret" . }}
{{- end }}
containers:
- - name: run-airflow-migrations
Review comment:
@mik-laj this code is already tested. If the migrations don't happen, the k8s tests never launch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org