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/11/05 18:40:39 UTC
[GitHub] [airflow] dimberman opened a new pull request #12114: Add Kubernetes files to selective checks
dimberman opened a new pull request #12114:
URL: https://github.com/apache/airflow/pull/12114
There are multiple kubernetes-related files that require
running the k8s integration tests. This PR adds those to the
run_selective_tests
<!--
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] potiuk commented on a change in pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#discussion_r518318874
##########
File path: scripts/ci/selective_ci_checks.sh
##########
@@ -475,8 +475,9 @@ function get_count_kubernetes_files() {
echo
local pattern_array=(
"^airflow/kubernetes"
Review comment:
```suggestion
```
----------------------------------------------------------------
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] potiuk commented on pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#issuecomment-722590565
> @potiuk I agree with @ashb here. Any change to the the kubernetes_executor should run all kube tests, but agreed that we don't need to worry if it's just k8sexecutor unit tests that are changed.
I agree here as well - no disagreement here. "kubernetes_executor" is part of core, so by definition it will run all 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] potiuk commented on a change in pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#discussion_r518289959
##########
File path: scripts/ci/selective_ci_checks.sh
##########
@@ -475,8 +475,11 @@ function get_count_kubernetes_files() {
echo
local pattern_array=(
"^airflow/kubernetes"
+ "^airflow/executors/kubernetes_executor.py"
"^chart"
- "^tests/kubernetes_tests"
+ "^tests/kubernetes"
Review comment:
This should not be there. Those "kubernetes_tests" are special tests that are not the same as all the "unit tests". They do not need CI image, instead they are using local virtual env to run tests and Kind-Kubernetes started on the host using the PROD image.
```suggestion
```
##########
File path: scripts/ci/selective_ci_checks.sh
##########
@@ -475,8 +475,11 @@ function get_count_kubernetes_files() {
echo
local pattern_array=(
"^airflow/kubernetes"
+ "^airflow/executors/kubernetes_executor.py"
"^chart"
- "^tests/kubernetes_tests"
+ "^tests/kubernetes"
+ "^kubernetes_tests"
+ "^tests/executors/test_kubernetes_executor.py"
Review comment:
```suggestion
```
Same here.
----------------------------------------------------------------
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 pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
dimberman commented on pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#issuecomment-722568884
@ashb I'm not sure that's necessary yet as there are no integration tests for the CeleryKubernetesExecutor yet.
----------------------------------------------------------------
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 edited a comment on pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
dimberman edited a comment on pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#issuecomment-722583481
@potiuk I agree with @ashb here. Any change to the the kubernetes_executor should run all kube tests, but agreed that we don't need to worry if it's just k8sexecutor unit tests that are changed.
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#discussion_r518318789
##########
File path: scripts/ci/selective_ci_checks.sh
##########
@@ -475,8 +475,9 @@ function get_count_kubernetes_files() {
echo
local pattern_array=(
"^airflow/kubernetes"
+ "^airflow/executors/kubernetes_executor.py"
Review comment:
It's harmful here. That will exclude ""^airflow/executors/kubernetes_executor.py" from "core".
The "core" of airflow is defined as everything that is not defined in on of the selection criteria before:
```
COUNT_CORE_OTHER_CHANGED_FILES=$((COUNT_ALL_CHANGED_FILES - COUNT_WWW_CHANGED_FILES - COUNT_PROVIDERS_CHANGED_FILES - COUNT_CLI_CHANGED_FILES - COUNT_API_CHANGED_FILES - COUNT_KUBERNETES_CHANGED_FILES))
```
This means that any "airflow/executors/kubernetes_executor.py" is treated as CORE/OTHER and it will trigger all tests (including Kubernetes tests). There are likely a number of unit tests depending on any change in the "airflow/executors" package, so if we change anything in this package, we have to run all tests
On the other hand, if ONLY files in "kubernetest_tests" change - we should only run kubernetes tests.
I reviewed it and there is another error here in fact We should not add "^airflow/kubernetes" here in the first place because there are some "kubernetes" unit tests (from "tests/kubernetes" folder) that could be skipped by accident if the "^airflow/kubernetes" only is changed).
----------------------------------------------------------------
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] potiuk edited a comment on pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#issuecomment-722590565
> @potiuk I agree with @ashb here. Any change to the the kubernetes_executor should run all kube tests, but agreed that we don't need to worry if it's just k8sexecutor unit tests that are changed.
I agree here as well - no disagreement here. "kubernetes_executor" is part of core, so by definition any change to it will run all 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] ashb commented on a change in pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#discussion_r518294941
##########
File path: scripts/ci/selective_ci_checks.sh
##########
@@ -475,8 +475,11 @@ function get_count_kubernetes_files() {
echo
local pattern_array=(
"^airflow/kubernetes"
+ "^airflow/executors/kubernetes_executor.py"
"^chart"
- "^tests/kubernetes_tests"
+ "^tests/kubernetes"
+ "^kubernetes_tests"
+ "^tests/executors/test_kubernetes_executor.py"
Review comment:
Changes to this file should run the Kube "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] ashb commented on pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#issuecomment-722582893
We need to add the executor code classes at least - changes to that file should run the Kube cluster 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] potiuk commented on a change in pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#discussion_r518318789
##########
File path: scripts/ci/selective_ci_checks.sh
##########
@@ -475,8 +475,9 @@ function get_count_kubernetes_files() {
echo
local pattern_array=(
"^airflow/kubernetes"
+ "^airflow/executors/kubernetes_executor.py"
Review comment:
It's harmful here. That will exclude ""^airflow/executors/kubernetes_executor.py" from "core".
The "core" of airflow is defined as everything that is not defined in on of the selection criteria before:
```
COUNT_CORE_OTHER_CHANGED_FILES=$((COUNT_ALL_CHANGED_FILES - COUNT_WWW_CHANGED_FILES - COUNT_PROVIDERS_CHANGED_FILES - COUNT_CLI_CHANGED_FILES - COUNT_API_CHANGED_FILES - COUNT_KUBERNETES_CHANGED_FILES))
```
This means that any "airflow/executors/kubernetes_executor.py" is treated as CORE/OTHER and it will trigger all tests (including kuberenetes tests). There are likely a number of unit tests depending on any change in "airflow/executors" package, so if we change anything in this package, we have to run all tests
On the other hand, if ONLY files in "kubernetest_tests" change - we should only run kubernetes tests.
There is another error here in fact We shoud not add "^airflow/lkubernetes" here in the first place because there are some "kubernetes" unit tests (from "tests/kubernetes" folder) that could be skipped by accident if the "^airflow/kubernetes" only is changed).
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#discussion_r518319638
##########
File path: scripts/ci/selective_ci_checks.sh
##########
@@ -475,8 +475,9 @@ function get_count_kubernetes_files() {
echo
local pattern_array=(
"^airflow/kubernetes"
+ "^airflow/executors/kubernetes_executor.py"
Review comment:
We should only live "chart" and "tests_kuberenetes" here. This will mean that when "charts" and "tests_kubernetes" to not change and no core changes the kubernetes tests will be skipped.
----------------------------------------------------------------
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 pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
dimberman commented on pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#issuecomment-722583481
@potiuk I agree with @ashb here. Any change to the the kubernetes_executor should run all kube 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] potiuk commented on a change in pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#discussion_r518318789
##########
File path: scripts/ci/selective_ci_checks.sh
##########
@@ -475,8 +475,9 @@ function get_count_kubernetes_files() {
echo
local pattern_array=(
"^airflow/kubernetes"
+ "^airflow/executors/kubernetes_executor.py"
Review comment:
It's harmful here. That will exclude ""^airflow/executors/kubernetes_executor.py" from "core".
The "core" of airflow is defined as everything that is not defined in on of the selection criteria before:
```
COUNT_CORE_OTHER_CHANGED_FILES=$((COUNT_ALL_CHANGED_FILES - COUNT_WWW_CHANGED_FILES - COUNT_PROVIDERS_CHANGED_FILES - COUNT_CLI_CHANGED_FILES - COUNT_API_CHANGED_FILES - COUNT_KUBERNETES_CHANGED_FILES))
```
This means that any "airflow/executors/kubernetes_executor.py" is treated as CORE/OTHER and it will trigger all tests (including kuberenetes tests). There are likely a number of unit tests depending on any change in "airflow/executors" package, so if we change anything in this package, we have to run all tests
On the other hand, if ONLY files in "kubernetest_tests" change - we should only run kubernetes tests.
I reviewed it and there is another error here in fact We shoud not add "^airflow/lkubernetes" here in the first place because there are some "kubernetes" unit tests (from "tests/kubernetes" folder) that could be skipped by accident if the "^airflow/kubernetes" only is changed).
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#discussion_r518319035
##########
File path: scripts/ci/selective_ci_checks.sh
##########
@@ -475,8 +475,9 @@ function get_count_kubernetes_files() {
echo
local pattern_array=(
"^airflow/kubernetes"
+ "^airflow/executors/kubernetes_executor.py"
Review comment:
```suggestion
```
----------------------------------------------------------------
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] github-actions[bot] commented on pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#issuecomment-722567501
The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#issuecomment-722607576
The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dimberman merged pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
dimberman merged pull request #12114:
URL: https://github.com/apache/airflow/pull/12114
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#discussion_r518319638
##########
File path: scripts/ci/selective_ci_checks.sh
##########
@@ -475,8 +475,9 @@ function get_count_kubernetes_files() {
echo
local pattern_array=(
"^airflow/kubernetes"
+ "^airflow/executors/kubernetes_executor.py"
Review comment:
We should only leave "chart" and "tests_kuberenetes" here. This will mean that when "charts" and "tests_kubernetes" to not change and no core changes the kubernetes tests will be skipped.
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#discussion_r518279713
##########
File path: scripts/ci/selective_ci_checks.sh
##########
@@ -475,8 +475,11 @@ function get_count_kubernetes_files() {
echo
local pattern_array=(
"^airflow/kubernetes"
+ "^airflow/executors/kubernetes_executor.py"
"^chart"
+ "^tests/kubernetes"
"^tests/kubernetes_tests"
Review comment:
```suggestion
"^kubernetes_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] potiuk commented on a change in pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#discussion_r518318789
##########
File path: scripts/ci/selective_ci_checks.sh
##########
@@ -475,8 +475,9 @@ function get_count_kubernetes_files() {
echo
local pattern_array=(
"^airflow/kubernetes"
+ "^airflow/executors/kubernetes_executor.py"
Review comment:
It's harmful here. That will exclude ""^airflow/executors/kubernetes_executor.py" from "core".
The "core" of airflow is defined as everything that is not defined in one of the selection criteria before:
```
COUNT_CORE_OTHER_CHANGED_FILES=$((COUNT_ALL_CHANGED_FILES - COUNT_WWW_CHANGED_FILES - COUNT_PROVIDERS_CHANGED_FILES - COUNT_CLI_CHANGED_FILES - COUNT_API_CHANGED_FILES - COUNT_KUBERNETES_CHANGED_FILES))
```
This means that any "airflow/executors/kubernetes_executor.py" is treated as CORE/OTHER and it will trigger all tests (including Kubernetes tests). There are likely a number of unit tests depending on any change in the "airflow/executors" package, so if we change anything in this package, we have to run all tests
On the other hand, if ONLY files in "kubernetest_tests" change - we should only run kubernetes tests.
I reviewed it and there is another error here in fact We should not add "^airflow/kubernetes" here in the first place because there are some "kubernetes" unit tests (from "tests/kubernetes" folder) that could be skipped by accident if the "^airflow/kubernetes" only is changed).
----------------------------------------------------------------
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] potiuk commented on a change in pull request #12114: Add Kubernetes files to selective checks
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12114:
URL: https://github.com/apache/airflow/pull/12114#discussion_r518319638
##########
File path: scripts/ci/selective_ci_checks.sh
##########
@@ -475,8 +475,9 @@ function get_count_kubernetes_files() {
echo
local pattern_array=(
"^airflow/kubernetes"
+ "^airflow/executors/kubernetes_executor.py"
Review comment:
We should only leave "chart" and "tests_kuberenetes" here. This will mean that when "charts" and "tests_kubernetes" do not change and there are no core changes - the kubernetes tests will be skipped.
Those are really "negative" selection criteria: If none of the files from the group changes - this group is skipped. Unless any core file changes, which triggers all 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