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 2022/03/24 11:15:47 UTC
[GitHub] [airflow] Bowrna opened a new pull request #22502: timeout added for docker operator
Bowrna opened a new pull request #22502:
URL: https://github.com/apache/airflow/pull/22502
Included the timeout support in APIClient of Docker in DockerOperator and Hook under Docker provider.
closes: #22115
related: #22115
<!--
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] Bowrna commented on pull request #22502: timeout added for docker operator
Posted by GitBox <gi...@apache.org>.
Bowrna commented on pull request #22502:
URL: https://github.com/apache/airflow/pull/22502#issuecomment-1079935196
> Some static checks/tests fail :).
Fixing it @potiuk
--
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] Bowrna commented on a change in pull request #22502: timeout added for docker operator
Posted by GitBox <gi...@apache.org>.
Bowrna commented on a change in pull request #22502:
URL: https://github.com/apache/airflow/pull/22502#discussion_r834201201
##########
File path: airflow/providers/docker/hooks/docker.py
##########
@@ -55,6 +55,7 @@ def __init__(
base_url: Optional[str] = None,
version: Optional[str] = None,
tls: Optional[str] = None,
+ timeout: int = 60,
Review comment:
@eladkal yes I could import the default from the package, that way it's easy to maintain. One other option I thought is to keep it None by default and not pass it at all if it's None. So that it will pick the default one when we don't pass to APIClient
--
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] eladkal commented on a change in pull request #22502: timeout added for docker operator
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #22502:
URL: https://github.com/apache/airflow/pull/22502#discussion_r834217025
##########
File path: airflow/providers/docker/hooks/docker.py
##########
@@ -55,6 +55,7 @@ def __init__(
base_url: Optional[str] = None,
version: Optional[str] = None,
tls: Optional[str] = None,
+ timeout: int = 60,
Review comment:
If setting to None falls to the default within the package then I guess it's prefered.
--
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] Bowrna commented on pull request #22502: Add timeout parameter to `DockerOperator`
Posted by GitBox <gi...@apache.org>.
Bowrna commented on pull request #22502:
URL: https://github.com/apache/airflow/pull/22502#issuecomment-1079946663
I see, let me fix the tests @potiuk @eladkal
--
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 merged pull request #22502: Add timeout parameter to `DockerOperator`
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #22502:
URL: https://github.com/apache/airflow/pull/22502
--
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] Bowrna commented on pull request #22502: Add timeout parameter to `DockerOperator`
Posted by GitBox <gi...@apache.org>.
Bowrna commented on pull request #22502:
URL: https://github.com/apache/airflow/pull/22502#issuecomment-1079941573
Static checks are fixed. While Postgres10 and Sqlite tests still fail. I am not sure how to fix those.
--
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 #22502: timeout added for docker operator
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22502:
URL: https://github.com/apache/airflow/pull/22502#issuecomment-1079930429
Some static checks/tests fail :).
--
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 #22502: Add timeout parameter to `DockerOperator`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22502:
URL: https://github.com/apache/airflow/pull/22502#issuecomment-1079945966
That's the case were tests **should** be changed reflect the new reality after the 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] Bowrna commented on pull request #22502: timeout added for docker operator
Posted by GitBox <gi...@apache.org>.
Bowrna commented on pull request #22502:
URL: https://github.com/apache/airflow/pull/22502#issuecomment-1077520285
While I have added timeout as specified in APIClient, I am not sure about adding test cases in the current test cases.
If anybody could suggest changes that I have to make in the test, it would be useful.
https://github.com/apache/airflow/blob/51d61df5a656101046a7825be53ac61ac4f2b047/tests/providers/docker/hooks/test_docker.py
--
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 #22502: Add timeout parameter to `DockerOperator`
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #22502:
URL: https://github.com/apache/airflow/pull/22502#issuecomment-1079945841
No. Default is fine. The tests have to be fixed because they don't expec the timeout parameter to be passed.
--
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] Bowrna commented on a change in pull request #22502: timeout added for docker operator
Posted by GitBox <gi...@apache.org>.
Bowrna commented on a change in pull request #22502:
URL: https://github.com/apache/airflow/pull/22502#discussion_r834210085
##########
File path: airflow/providers/docker/hooks/docker.py
##########
@@ -55,6 +55,7 @@ def __init__(
base_url: Optional[str] = None,
version: Optional[str] = None,
tls: Optional[str] = None,
+ timeout: int = 60,
Review comment:
@eladkal which one do you think will work best? Keeping None as default or passing the default of package by importing?
--
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 #22502: timeout added for docker operator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #22502:
URL: https://github.com/apache/airflow/pull/22502#issuecomment-1079923932
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] eladkal commented on pull request #22502: Add timeout parameter to `DockerOperator`
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #22502:
URL: https://github.com/apache/airflow/pull/22502#issuecomment-1079942018
It fails because there are assertion errors:
https://github.com/apache/airflow/runs/5710010118?check_suite_focus=true#step:9:7584
Something with how we manage the default value isn't quite right (I think)
--
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] eladkal commented on pull request #22502: timeout added for docker operator
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #22502:
URL: https://github.com/apache/airflow/pull/22502#issuecomment-1079212952
Can you fix the failing 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Bowrna commented on a change in pull request #22502: timeout added for docker operator
Posted by GitBox <gi...@apache.org>.
Bowrna commented on a change in pull request #22502:
URL: https://github.com/apache/airflow/pull/22502#discussion_r834217701
##########
File path: airflow/providers/docker/hooks/docker.py
##########
@@ -55,6 +55,7 @@ def __init__(
base_url: Optional[str] = None,
version: Optional[str] = None,
tls: Optional[str] = None,
+ timeout: int = 60,
Review comment:
Ok @eladkal then i will verify it and make relevant changes
--
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] eladkal commented on a change in pull request #22502: timeout added for docker operator
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #22502:
URL: https://github.com/apache/airflow/pull/22502#discussion_r834199244
##########
File path: airflow/providers/docker/hooks/docker.py
##########
@@ -55,6 +55,7 @@ def __init__(
base_url: Optional[str] = None,
version: Optional[str] = None,
tls: Optional[str] = None,
+ timeout: int = 60,
Review comment:
can/should we import the default of the package rather than hard code value from our side?
https://github.com/docker/docker-py/blob/a48a5a9647761406d66e8271f19fab7fa0c5f582/docker/constants.py#L6
--
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] Bowrna commented on a change in pull request #22502: timeout added for docker operator
Posted by GitBox <gi...@apache.org>.
Bowrna commented on a change in pull request #22502:
URL: https://github.com/apache/airflow/pull/22502#discussion_r835317194
##########
File path: airflow/providers/docker/hooks/docker.py
##########
@@ -55,6 +55,7 @@ def __init__(
base_url: Optional[str] = None,
version: Optional[str] = None,
tls: Optional[str] = None,
+ timeout: int = 60,
Review comment:
@eladkal I have added default within package by importing it as default param.
--
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] eladkal commented on a change in pull request #22502: Add timeout parameter to `DockerOperator`
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #22502:
URL: https://github.com/apache/airflow/pull/22502#discussion_r836704176
##########
File path: tests/providers/docker/operators/test_docker.py
##########
@@ -22,6 +22,7 @@
import pytest
from docker.errors import APIError
+from docker.constants import DEFAULT_TIMEOUT_SECONDS
Review comment:
```suggestion
from docker.constants import DEFAULT_TIMEOUT_SECONDS
from docker.errors import APIError
```
to fix static checks
--
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