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/04/07 11:45:16 UTC
[GitHub] [airflow] akki opened a new pull request #14960: Support all terminus task states in Docker Swarm Operator
akki opened a new pull request #14960:
URL: https://github.com/apache/airflow/pull/14960
Docker reference for task states - https://docs.docker.com/engine/swarm/how-swarm-mode-works/swarm-task-states/
In case of existing issue, reference it using one of the following:
closes: https://github.com/apache/airflow/issues/14959
--
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] akki edited a comment on pull request #14960: Support all terminus task states in Docker Swarm Operator
Posted by GitBox <gi...@apache.org>.
akki edited a comment on pull request #14960:
URL: https://github.com/apache/airflow/pull/14960#issuecomment-814708219
@eladkal I am sorry if I am missing something again but the CI errors seem to change everytime I (force) pushed the same code.
Also, in the errors, I don't really see any errors. I just says "Upload artifact for coverage". Last time it had failed on "quarantine tests" step but this time it failed on the "Upload container logs" step (when I pushed the exact same code). May I request your help to figure out how I can fix this?
<img width="899" alt="Support_all_terminus_task_states_in_Docker_Swarm_Operator_by_akki_·_Pull_Request__14960_·_apache_airflow" src="https://user-images.githubusercontent.com/4801628/113839235-b5d04000-97b9-11eb-820c-e539f20feefb.png">
--
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] eladkal commented on a change in pull request #14960: Support all terminus task states in Docker Swarm Operator
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #14960:
URL: https://github.com/apache/airflow/pull/14960#discussion_r606830147
##########
File path: tests/providers/docker/operators/test_docker_swarm.py
##########
@@ -165,7 +165,7 @@ def test_failed_service_raises_error(self, types_mock, client_class_mock):
client_class_mock.return_value = client_mock
operator = DockerSwarmOperator(image='', auto_remove=False, task_id='unittest', enable_logging=False)
- msg = "Service failed: {'ID': 'some_id'}"
+ msg = "Service did not complete: {'ID': 'some_id'}"
Review comment:
Currently the operator has coverage only for `complete`/`failed` statuses.
Since this PR adds more statuses should we maybe rename this test from `test_failed_service_raises_error` to `test_failure_service_raises_error` and make it parametrized with `['failed', 'shutdown', 'rejected', 'orphaned', 'remove']` ?
--
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] eladkal merged pull request #14960: Support all terminus task states in Docker Swarm Operator
Posted by GitBox <gi...@apache.org>.
eladkal merged pull request #14960:
URL: https://github.com/apache/airflow/pull/14960
--
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] akki commented on pull request #14960: Support all terminus task states in Docker Swarm Operator
Posted by GitBox <gi...@apache.org>.
akki commented on pull request #14960:
URL: https://github.com/apache/airflow/pull/14960#issuecomment-811977540
Hi
Can any of the admins please review this PR. I would also like to know anything else that is required to get this code merged.
Also, the database build failure doesn't seem related to my changes. I also see the same issue with [many](https://github.com/apache/airflow/pull/15122) [other](https://github.com/apache/airflow/pull/15123) [PRs](https://github.com/apache/airflow/pull/15104) recently, so I guess the check is broken. Please advise.
<img width="1142" alt="check_for_zipped_files_in_import_rules_for_upgrade_check_by_ShakaibKhan_·_Pull_Request__15123_·_apache_airflow" src="https://user-images.githubusercontent.com/4801628/113315570-9fe4fa00-9337-11eb-883b-5f0757761b46.png">
Thanks
--
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] akki commented on pull request #14960: Support all terminus task states in Docker Swarm Operator
Posted by GitBox <gi...@apache.org>.
akki commented on pull request #14960:
URL: https://github.com/apache/airflow/pull/14960#issuecomment-813067412
@eladkal
Thank you so much for pointing and sorry for the test failure.
I have fixed the test cases & CI also seems happy now.
--
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] akki commented on a change in pull request #14960: Support all terminus task states in Docker Swarm Operator
Posted by GitBox <gi...@apache.org>.
akki commented on a change in pull request #14960:
URL: https://github.com/apache/airflow/pull/14960#discussion_r606923512
##########
File path: tests/providers/docker/operators/test_docker_swarm.py
##########
@@ -165,7 +165,7 @@ def test_failed_service_raises_error(self, types_mock, client_class_mock):
client_class_mock.return_value = client_mock
operator = DockerSwarmOperator(image='', auto_remove=False, task_id='unittest', enable_logging=False)
- msg = "Service failed: {'ID': 'some_id'}"
+ msg = "Service did not complete: {'ID': 'some_id'}"
Review comment:
Thanks for the suggestion. Definitely, I have pushed the required changes.
Please have a look.
--
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 #14960: Support all terminus task states in Docker Swarm Operator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14960:
URL: https://github.com/apache/airflow/pull/14960#issuecomment-813068971
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 master 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal closed pull request #14960: Support all terminus task states in Docker Swarm Operator
Posted by GitBox <gi...@apache.org>.
eladkal closed pull request #14960:
URL: https://github.com/apache/airflow/pull/14960
--
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] eladkal commented on pull request #14960: Support all terminus task states in Docker Swarm Operator
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #14960:
URL: https://github.com/apache/airflow/pull/14960#issuecomment-811987579
@akki the failures are on docker swarm tests:
```
___________ TestDockerSwarmOperator.test_failed_service_raises_error ___________
self = <tests.providers.docker.operators.test_docker_swarm.TestDockerSwarmOperator testMethod=test_failed_service_raises_error>
types_mock = <MagicMock name='types' id='140504999858416'>
client_class_mock = <MagicMock name='APIClient' id='140504999859312'>
@mock.patch('airflow.providers.docker.operators.docker.APIClient')
@mock.patch('airflow.providers.docker.operators.docker_swarm.types')
def test_failed_service_raises_error(self, types_mock, client_class_mock):
mock_obj = mock.Mock()
client_mock = mock.Mock(spec=APIClient)
client_mock.create_service.return_value = {'ID': 'some_id'}
client_mock.images.return_value = []
client_mock.pull.return_value = [b'{"status":"pull log"}']
client_mock.tasks.return_value = [{'Status': {'State': 'failed'}}]
types_mock.TaskTemplate.return_value = mock_obj
types_mock.ContainerSpec.return_value = mock_obj
types_mock.RestartPolicy.return_value = mock_obj
types_mock.Resources.return_value = mock_obj
client_class_mock.return_value = client_mock
operator = DockerSwarmOperator(image='', auto_remove=False, task_id='unittest', enable_logging=False)
msg = "Service failed: {'ID': 'some_id'}"
with pytest.raises(AirflowException) as ctx:
operator.execute(None)
> assert str(ctx.value) == msg
E AssertionError: assert "Service did ...': 'some_id'}" == "Service fail...': 'some_id'}"
E - Service failed: {'ID': 'some_id'}
E ? ^^ ^
E + Service did not complete: {'ID': 'some_id'}
E ? ^ ++++++++++ ^^
tests/providers/docker/operators/test_docker_swarm.py:171: AssertionError
```
--
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] akki commented on pull request #14960: Support all terminus task states in Docker Swarm Operator
Posted by GitBox <gi...@apache.org>.
akki commented on pull request #14960:
URL: https://github.com/apache/airflow/pull/14960#issuecomment-814708219
@eladkal I am sorry if I am missing something again but the CI errors seem to change everytime I (force) pushed the same code.
Also, in the errors, I don't really see any errors. I just says "Upload artifact for coverage". Last time it had failed on "quarantine tests" step but this time it failed on the "Upload container logs" step (when I pushed the exact same code). May I request your help to figure out how I can fix this?
--
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] akki commented on a change in pull request #14960: Support all terminus task states in Docker Swarm Operator
Posted by GitBox <gi...@apache.org>.
akki commented on a change in pull request #14960:
URL: https://github.com/apache/airflow/pull/14960#discussion_r608436757
##########
File path: tests/providers/docker/operators/test_docker_swarm.py
##########
@@ -148,27 +148,31 @@ def test_no_auto_remove(self, types_mock, client_class_mock):
@mock.patch('airflow.providers.docker.operators.docker.APIClient')
@mock.patch('airflow.providers.docker.operators.docker_swarm.types')
- def test_failed_service_raises_error(self, types_mock, client_class_mock):
+ def test_non_complete_service_raises_error(self, types_mock, client_class_mock):
mock_obj = mock.Mock()
client_mock = mock.Mock(spec=APIClient)
client_mock.create_service.return_value = {'ID': 'some_id'}
client_mock.images.return_value = []
client_mock.pull.return_value = [b'{"status":"pull log"}']
- client_mock.tasks.return_value = [{'Status': {'State': 'failed'}}]
types_mock.TaskTemplate.return_value = mock_obj
types_mock.ContainerSpec.return_value = mock_obj
types_mock.RestartPolicy.return_value = mock_obj
types_mock.Resources.return_value = mock_obj
client_class_mock.return_value = client_mock
- operator = DockerSwarmOperator(image='', auto_remove=False, task_id='unittest', enable_logging=False)
+ non_complete_statuses = ['failed', 'shutdown', 'rejected', 'orphaned', 'remove']
msg = "Service did not complete: {'ID': 'some_id'}"
- with pytest.raises(AirflowException) as ctx:
- operator.execute(None)
- assert str(ctx.value) == msg
+ for status in non_complete_statuses:
Review comment:
Done.
--
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] eladkal commented on a change in pull request #14960: Support all terminus task states in Docker Swarm Operator
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #14960:
URL: https://github.com/apache/airflow/pull/14960#discussion_r606923303
##########
File path: tests/providers/docker/operators/test_docker_swarm.py
##########
@@ -148,27 +148,31 @@ def test_no_auto_remove(self, types_mock, client_class_mock):
@mock.patch('airflow.providers.docker.operators.docker.APIClient')
@mock.patch('airflow.providers.docker.operators.docker_swarm.types')
- def test_failed_service_raises_error(self, types_mock, client_class_mock):
+ def test_non_complete_service_raises_error(self, types_mock, client_class_mock):
mock_obj = mock.Mock()
client_mock = mock.Mock(spec=APIClient)
client_mock.create_service.return_value = {'ID': 'some_id'}
client_mock.images.return_value = []
client_mock.pull.return_value = [b'{"status":"pull log"}']
- client_mock.tasks.return_value = [{'Status': {'State': 'failed'}}]
types_mock.TaskTemplate.return_value = mock_obj
types_mock.ContainerSpec.return_value = mock_obj
types_mock.RestartPolicy.return_value = mock_obj
types_mock.Resources.return_value = mock_obj
client_class_mock.return_value = client_mock
- operator = DockerSwarmOperator(image='', auto_remove=False, task_id='unittest', enable_logging=False)
+ non_complete_statuses = ['failed', 'shutdown', 'rejected', 'orphaned', 'remove']
msg = "Service did not complete: {'ID': 'some_id'}"
- with pytest.raises(AirflowException) as ctx:
- operator.execute(None)
- assert str(ctx.value) == msg
+ for status in non_complete_statuses:
Review comment:
can you convert the loop to` parameterized.expand` ?
see examples:
https://github.com/apache/airflow/blob/e86f5ca8fa5ff22c1e1f48addc012919034c672f/tests/providers/amazon/aws/sensors/test_step_function_execution.py#L52
https://github.com/apache/airflow/blob/0f327788b5b0887c463cb83dd8f732245da96577/tests/providers/tableau/sensors/test_tableau_job_status.py#L53
--
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