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/11/18 11:25:00 UTC
[GitHub] [airflow] ashb opened a new pull request #19676: Use built-in check-migrations command for Airflow>=2 in helm chart
ashb opened a new pull request #19676:
URL: https://github.com/apache/airflow/pull/19676
<!--
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/main/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/main/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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb merged pull request #19676: Use built-in check-migrations command for Airflow>=2 in helm chart
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #19676:
URL: https://github.com/apache/airflow/pull/19676
--
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] ashb commented on pull request #19676: Use built-in check-migrations command for Airflow>=2 in helm chart
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19676:
URL: https://github.com/apache/airflow/pull/19676#issuecomment-972800835
``` E AssertionError: Could not enable DAG: {'detail': None, 'status': 404, 'title': "Dag with id: 'example_kubernetes_executor_config' not found", 'type': 'http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/apache-airflow/latest/stable-rest-api-ref.html#section/Errors/NotFound'}```
Curious. I wonder why only some of the helm chart matrix failed with that.
--
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] kaxil commented on pull request #19676: Use built-in check-migrations command for Airflow>=2 in helm chart
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #19676:
URL: https://github.com/apache/airflow/pull/19676#issuecomment-972819296
> ` E AssertionError: Could not enable DAG: {'detail': None, 'status': 404, 'title': "Dag with id: 'example_kubernetes_executor_config' not found", 'type': 'http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/apache-airflow/latest/stable-rest-api-ref.html#section/Errors/NotFound'}`
>
> Curious. I wonder why only some of the helm chart matrix failed with that.
Oh !!! 🤦 We deleted that DAG and added everything `example_kubernetes_executor`
--
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 #19676: Use built-in check-migrations command for Airflow>=2 in helm chart
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #19676:
URL: https://github.com/apache/airflow/pull/19676#issuecomment-972859786
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.
--
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] kaxil commented on pull request #19676: Use built-in check-migrations command for Airflow>=2 in helm chart
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #19676:
URL: https://github.com/apache/airflow/pull/19676#issuecomment-972820082
Need:
```
diff --git a/kubernetes_tests/test_kubernetes_executor.py b/kubernetes_tests/test_kubernetes_executor.py
index 1ca3848fe..61030d1b9 100644
--- a/kubernetes_tests/test_kubernetes_executor.py
+++ b/kubernetes_tests/test_kubernetes_executor.py
@@ -25,7 +25,7 @@ from kubernetes_tests.test_base import EXECUTOR, TestBase
@pytest.mark.skipif(EXECUTOR != 'KubernetesExecutor', reason="Only runs on KubernetesExecutor")
class TestKubernetesExecutor(TestBase):
def test_integration_run_dag(self):
- dag_id = 'example_kubernetes_executor_config'
+ dag_id = 'example_kubernetes_executor'
dag_run_id, execution_date = self.start_job_in_kubernetes(dag_id, self.host)
print(f"Found the job with execution_date {execution_date}")
@@ -48,7 +48,7 @@ class TestKubernetesExecutor(TestBase):
)
def test_integration_run_dag_with_scheduler_failure(self):
- dag_id = 'example_kubernetes_executor_config'
+ dag_id = 'example_kubernetes_executor'
dag_run_id, execution_date = self.start_job_in_kubernetes(dag_id, self.host)
```
But we can create a separate PR for that
--
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] ashb commented on pull request #19676: Use built-in check-migrations command for Airflow>=2 in helm chart
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19676:
URL: https://github.com/apache/airflow/pull/19676#issuecomment-972844172
Turns out living with broken tests is bad -- who'da thunk it!?
--
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] kaxil edited a comment on pull request #19676: Use built-in check-migrations command for Airflow>=2 in helm chart
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #19676:
URL: https://github.com/apache/airflow/pull/19676#issuecomment-972820082
Need:
```diff
diff --git a/kubernetes_tests/test_kubernetes_executor.py b/kubernetes_tests/test_kubernetes_executor.py
index 1ca3848fe..61030d1b9 100644
--- a/kubernetes_tests/test_kubernetes_executor.py
+++ b/kubernetes_tests/test_kubernetes_executor.py
@@ -25,7 +25,7 @@ from kubernetes_tests.test_base import EXECUTOR, TestBase
@pytest.mark.skipif(EXECUTOR != 'KubernetesExecutor', reason="Only runs on KubernetesExecutor")
class TestKubernetesExecutor(TestBase):
def test_integration_run_dag(self):
- dag_id = 'example_kubernetes_executor_config'
+ dag_id = 'example_kubernetes_executor'
dag_run_id, execution_date = self.start_job_in_kubernetes(dag_id, self.host)
print(f"Found the job with execution_date {execution_date}")
@@ -48,7 +48,7 @@ class TestKubernetesExecutor(TestBase):
)
def test_integration_run_dag_with_scheduler_failure(self):
- dag_id = 'example_kubernetes_executor_config'
+ dag_id = 'example_kubernetes_executor'
dag_run_id, execution_date = self.start_job_in_kubernetes(dag_id, self.host)
```
But we can create a separate PR for that
--
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] ashb commented on a change in pull request #19676: Use built-in check-migrations command for Airflow>=2 in helm chart
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #19676:
URL: https://github.com/apache/airflow/pull/19676#discussion_r752226402
##########
File path: chart/templates/_helpers.yaml
##########
@@ -542,7 +542,11 @@ Create the name of the cleanup service account to use
{{- end -}}
{{ define "wait-for-migrations-command" }}
- {{/* From Airflow 2.0.0 this can become [airflow, db, check-migrations] */}}
+ {{- if semverCompare ">=2.0.0" .Values.airflowVersion }}
Review comment:
Test added (and one for < 2.0 condition too.)
--
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] ashb commented on pull request #19676: Use built-in check-migrations command for Airflow>=2 in helm chart
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19676:
URL: https://github.com/apache/airflow/pull/19676#issuecomment-972847990
https://github.com/apache/airflow/pull/19680
--
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 #19676: Use built-in check-migrations command for Airflow>=2 in helm chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #19676:
URL: https://github.com/apache/airflow/pull/19676#issuecomment-972962956
Nice!. Just got back to my PC :)
--
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] kaxil commented on a change in pull request #19676: Use built-in check-migrations command for Airflow>=2 in helm chart
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #19676:
URL: https://github.com/apache/airflow/pull/19676#discussion_r752161590
##########
File path: chart/templates/_helpers.yaml
##########
@@ -542,7 +542,11 @@ Create the name of the cleanup service account to use
{{- end -}}
{{ define "wait-for-migrations-command" }}
- {{/* From Airflow 2.0.0 this can become [airflow, db, check-migrations] */}}
+ {{- if semverCompare ">=2.0.0" .Values.airflowVersion }}
Review comment:
Should we add a simple test like the following:
```diff
diff --git a/chart/tests/test_webserver.py b/chart/tests/test_webserver.py
index 329e6117d..6f92af2d6 100644
--- a/chart/tests/test_webserver.py
+++ b/chart/tests/test_webserver.py
@@ -137,6 +137,21 @@ class WebserverDeploymentTest(unittest.TestCase):
"image": "test-registry/test-repo:test-tag",
} == jmespath.search("spec.template.spec.containers[-1]", docs[0])
+ @parameterized.expand(
+ [
+ ("2.0.0", ["airflow", "db", "check-migrations"]),
+ ("2.1.0", ["airflow", "db", "check-migrations"]),
+ ],
+ )
+ def test_wait_for_migration_airflow_version(self, airflow_version, expected_arg):
+ docs = render_chart(
+ values={
+ "airflowVersion": airflow_version,
+ },
+ show_only=["templates/webserver/webserver-deployment.yaml"],
+ )
+ assert expected_arg == jmespath.search("spec.template.spec.initContainers[0].args", docs[0])
+
def test_should_add_extra_init_containers(self):
docs = render_chart(
values={
```
--
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] ashb commented on pull request #19676: Use built-in check-migrations command for Airflow>=2 in helm chart
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #19676:
URL: https://github.com/apache/airflow/pull/19676#issuecomment-972800168
Well _some_ of the Helm chart tests passed, that's promising.
--
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