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/04/11 09:41:30 UTC
[GitHub] [airflow] joppevos opened a new pull request, #22901: Add exception in Breeze when docker is not running
joppevos opened a new pull request, #22901:
URL: https://github.com/apache/airflow/pull/22901
Running the new python Breeze environment crashes, if Docker is not running.
This PR will give a descriptive error.
Traceback of the error:
```
File "/Users/vosjoppe/github/airflow/dev/breeze/src/airflow_breeze/breeze.py", line 707, in shell
enter_shell(
File "/Users/vosjoppe/github/airflow/dev/breeze/src/airflow_breeze/shell/enter_shell.py", line 193, in enter_shell
check_docker_version(verbose)
File "/Users/vosjoppe/github/airflow/dev/breeze/src/airflow_breeze/utils/docker_command_utils.py", line 181, in check_docker_version
if docker_version_output.returncode == 0:
AttributeError: 'NoneType' object has no attribute 'returncode'
```
<!--
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**
--
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 a diff in pull request #22901: Catch error in Breeze when docker is not running
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22901:
URL: https://github.com/apache/airflow/pull/22901#discussion_r847197689
##########
dev/breeze/src/airflow_breeze/utils/docker_command_utils.py:
##########
@@ -179,11 +179,7 @@ def check_docker_version(verbose: bool):
text=True,
)
if not docker_version_output:
- console.print(
- '[red]ERROR[/] Docker version could not be determined. Please make sure Docker is installed '
- 'and running.'
- )
- return
+ raise Exception("No running Docker instance found")
Review Comment:
Added https://github.com/apache/airflow/issues/22906 to describe the rules (but in the meantime, please follow those and make the message nicer :))
--
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] joppevos commented on pull request #22901: Add exception in Breeze when docker is not running
Posted by GitBox <gi...@apache.org>.
joppevos commented on PR #22901:
URL: https://github.com/apache/airflow/pull/22901#issuecomment-1094825881
We could also stop breeze from starting up with an exception like so
```python
raise Exception("No running Docker instance found")
```
But I think it is clear enough to the user, since breeze runs it this error later anyway
```shell
Skip mounting host volumes to Docker
docker: Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?.
```
--
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 a diff in pull request #22901: Catch error in Breeze when docker is not running
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22901:
URL: https://github.com/apache/airflow/pull/22901#discussion_r847195377
##########
dev/breeze/src/airflow_breeze/utils/docker_command_utils.py:
##########
@@ -179,11 +179,7 @@ def check_docker_version(verbose: bool):
text=True,
)
if not docker_version_output:
- console.print(
- '[red]ERROR[/] Docker version could not be determined. Please make sure Docker is installed '
- 'and running.'
- )
- return
+ raise Exception("No running Docker instance found")
Review Comment:
Yeah. This is a CLI tool so at any point when we raise exception, it should only happen when there are "unknown" error conditions.
What we usually do in other parts of the breeze code is:
1) use rich's console (from utils)
2) write the error with `[red[/]"
3) provide helpful guidance of what to do in `[bright_yellpw][/]" markers
4) sys.exit(1) (or other non-zero error)
I have not documented this yet, but we might want to add ADR for that and do some custom rich "styles" (info/error/guidance etc.) to make it more obvious. I will add an issue 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] joppevos commented on pull request #22901: Catch error in Breeze when docker is not running
Posted by GitBox <gi...@apache.org>.
joppevos commented on PR #22901:
URL: https://github.com/apache/airflow/pull/22901#issuecomment-1094852169
> Personally I’d prefer breeze to end early. There’s a bunch of output (including a giant ASCII part) after this check and we could avoid all those easily.
You are right. It is better to end it early. I pushed the 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] potiuk commented on a diff in pull request #22901: Catch error in Breeze when docker is not running
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22901:
URL: https://github.com/apache/airflow/pull/22901#discussion_r847195377
##########
dev/breeze/src/airflow_breeze/utils/docker_command_utils.py:
##########
@@ -179,11 +179,7 @@ def check_docker_version(verbose: bool):
text=True,
)
if not docker_version_output:
- console.print(
- '[red]ERROR[/] Docker version could not be determined. Please make sure Docker is installed '
- 'and running.'
- )
- return
+ raise Exception("No running Docker instance found")
Review Comment:
Yeah. This is a CLI tool so at any point when we raise exception, it should only happen when there are "unknown" error conditions.
What we usually do in other parts of the breeze code when we have a "known" exception is:
1) use rich's console (from utils)
2) write the error with `[red[/]"
3) provide helpful guidance of what to do in `[bright_yellpw][/]" markers
4) sys.exit(1) (or other non-zero error)
I have not documented this yet, but we might want to add ADR for that and do some custom rich "styles" (info/error/guidance etc.) to make it more obvious. I will add an issue 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] uranusjr commented on pull request #22901: Catch error in Breeze when docker is not running
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #22901:
URL: https://github.com/apache/airflow/pull/22901#issuecomment-1094831354
Personally I’d prefer breeze to end early. There’s a bunch of output (including a giant ASCII part) after this check and we could avoid all those easily.
--
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 #22901: Catch error in Breeze when docker is not running
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22901:
URL: https://github.com/apache/airflow/pull/22901#issuecomment-1095088933
The PR is likely ready to be merged. No tests are needed as no important environment files, nor python files were modified by it. However, committers might decide that full test matrix is needed and add the 'full tests needed' label. Then you should rebase it 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] joppevos commented on a diff in pull request #22901: Catch error in Breeze when docker is not running
Posted by GitBox <gi...@apache.org>.
joppevos commented on code in PR #22901:
URL: https://github.com/apache/airflow/pull/22901#discussion_r847235044
##########
dev/breeze/src/airflow_breeze/utils/docker_command_utils.py:
##########
@@ -179,11 +179,7 @@ def check_docker_version(verbose: bool):
text=True,
)
if not docker_version_output:
- console.print(
- '[red]ERROR[/] Docker version could not be determined. Please make sure Docker is installed '
- 'and running.'
- )
- return
+ raise Exception("No running Docker instance found")
Review Comment:
Thanks for the information.
I implemented the changes. We now have a more graceful and helpful message :)
```shell
> python3 dev/breeze/src/airflow_breeze/breeze.py
Docker is not running.
Please make sure Docker is installed and running.
```
--
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 #22901: Catch error in Breeze when docker is not running
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22901:
URL: https://github.com/apache/airflow/pull/22901#issuecomment-1094905422
BTW. Cool that we start gettiing 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 merged pull request #22901: Catch error in Breeze when docker is not running
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #22901:
URL: https://github.com/apache/airflow/pull/22901
--
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] uranusjr commented on a diff in pull request #22901: Catch error in Breeze when docker is not running
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #22901:
URL: https://github.com/apache/airflow/pull/22901#discussion_r847173433
##########
dev/breeze/src/airflow_breeze/utils/docker_command_utils.py:
##########
@@ -179,11 +179,7 @@ def check_docker_version(verbose: bool):
text=True,
)
if not docker_version_output:
- console.print(
- '[red]ERROR[/] Docker version could not be determined. Please make sure Docker is installed '
- 'and running.'
- )
- return
+ raise Exception("No running Docker instance found")
Review Comment:
Instead of failing with an ugly-ish traceback, can we catch this in the caller and exit more gracefully instead?
--
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