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/23 16:04:36 UTC
[GitHub] [airflow] potiuk opened a new pull request, #23189: Fix and improve consistency of checking command return code
potiuk opened a new pull request, #23189:
URL: https://github.com/apache/airflow/pull/23189
This is an aftermath of #23104 after switchig to docs building
by breeze, failure of build documentation did not trigger failure
of the docs build (but it did trigger main failure of pushing
the documentation).
This change improves and simplifies the return code processing and
propagation in the commands executed by breeze - thanks to common
returncode, stdout, stderr available in both CompletedProcess
and CalledProcessError and returning fake CompletedProcess in dry_run
mode, we can also satisfy MyPy type check by returning non-optional
Union of those two types which simplifies returncode processing.
This change fixes the error in the docs (lack of empty lines before
auto-generated extras).
All commands have been reviewed to see if the returncode is
correctly handled where needed.
<!--
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 a newsfragement file, named `{pr_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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 #23189: Fix and improve consistency of checking command return code
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23189:
URL: https://github.com/apache/airflow/pull/23189#discussion_r856938949
##########
dev/breeze/src/airflow_breeze/utils/run_utils.py:
##########
@@ -91,9 +91,9 @@ def run_command(
console.print("[red]========================= STDERR start ============================[/]")
console.print(ex.stderr)
console.print("[red]========================= STDERR end ==============================[/]")
- if not check:
+ if check:
raise
- return None
+ return ex
Review Comment:
I just realised this today when I looked at the "failing but green docs" and I realized this Union is cool.
--
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 #23189: Fix and improve consistency of checking command return code
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #23189:
URL: https://github.com/apache/airflow/pull/23189
--
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 #23189: Fix and improve consistency of checking command return code
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23189:
URL: https://github.com/apache/airflow/pull/23189#issuecomment-1107532338
Fixed unit tests 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.
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 diff in pull request #23189: Fix and improve consistency of checking command return code
Posted by GitBox <gi...@apache.org>.
eladkal commented on code in PR #23189:
URL: https://github.com/apache/airflow/pull/23189#discussion_r856937318
##########
dev/breeze/src/airflow_breeze/utils/run_utils.py:
##########
@@ -91,9 +91,9 @@ def run_command(
console.print("[red]========================= STDERR start ============================[/]")
console.print(ex.stderr)
console.print("[red]========================= STDERR end ==============================[/]")
- if not check:
+ if check:
raise
- return None
+ return ex
Review Comment:
why do we need to return the exception?
--
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 #23189: Fix and improve consistency of checking command return code
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23189:
URL: https://github.com/apache/airflow/pull/23189#issuecomment-1107533108
BTW. You can see the "effect" of this problem
https://github.com/apache/airflow/runs/6140263239?check_suite_focus=true#step:11:144 -> static checks failed but the job was green 😱 😱 😱 😱 😱
--
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 #23189: Fix and improve consistency of checking command return code
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23189:
URL: https://github.com/apache/airflow/pull/23189#issuecomment-1107532771
It also made it slightly easier for "dry-run" mode. All our commands have `dry-run` option which will print what breeze is about to execute and allow to do copy-paste and run it manually. We returned None, but now instead we create an aritifficial CompletedProcess with return_code = 0 - so some conditional handling has been simplified due to that as well.
--
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 #23189: Fix and improve consistency of checking command return code
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23189:
URL: https://github.com/apache/airflow/pull/23189#discussion_r856939108
##########
dev/breeze/src/airflow_breeze/utils/run_utils.py:
##########
@@ -91,9 +91,9 @@ def run_command(
console.print("[red]========================= STDERR start ============================[/]")
console.print(ex.stderr)
console.print("[red]========================= STDERR end ==============================[/]")
- if not check:
+ if check:
raise
- return None
+ return ex
Review Comment:
BTW. I will add ADR about it i.e "How we are handling communication and errors in Breeze" - I have a task for it in https://github.com/apache/airflow/issues/22906
--
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 #23189: Fix and improve consistency of checking command return code
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23189:
URL: https://github.com/apache/airflow/pull/23189#discussion_r856938871
##########
dev/breeze/src/airflow_breeze/utils/run_utils.py:
##########
@@ -91,9 +91,9 @@ def run_command(
console.print("[red]========================= STDERR start ============================[/]")
console.print(ex.stderr)
console.print("[red]========================= STDERR end ==============================[/]")
- if not check:
+ if check:
raise
- return None
+ return ex
Review Comment:
Because `CalledProcessorError` has the same fields as `CompletedProcess` :)
Both have:
* returncode
* stdout
* stderr
And then, we can handle it much easier (and it makes handling returned types much easier when we use `check=False`) - instead of `Optional[CompletedProcess]` we return now `Union[CalledProcessError,CompletedProcess`]. Then we can do:
```
if result.returncode != 0:
......
sys.exit(result.returncode)
```
instead of something like that:
```
if not result or returncode != 0:
......
sys.exit(result.returncode if result else 1)
```
This makes all the conditionals far easier.
One could ask why we need to get returncode and `check = True`. The main reason why we do it because this is a "command line tool".
So rather than printing the user an ugly stacktrace we really want to print a nice and explanatory error message (nice with [red] marker) so we just need to exit with the right returncode rather than throw ugly exception. This is in cases where we "expect" some errors (for example we expect that "building docs" might fail - but we already handle it by printing all reasons and errors.
We still have a number of cases when we do "check=True" - this is when we do not expect the command to fail ever (but for many reasons - misconfigured system etc it might happen). In this case we simply run `check=True` and if such command fails, ugly exception will be printed (ugly but useful for diagnostics in this case :).
--
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 #23189: Fix and improve consistency of checking command return code
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23189:
URL: https://github.com/apache/airflow/pull/23189#discussion_r856939108
##########
dev/breeze/src/airflow_breeze/utils/run_utils.py:
##########
@@ -91,9 +91,9 @@ def run_command(
console.print("[red]========================= STDERR start ============================[/]")
console.print(ex.stderr)
console.print("[red]========================= STDERR end ==============================[/]")
- if not check:
+ if check:
raise
- return None
+ return ex
Review Comment:
BTW. I will add some adr about it i.e "How we are handling communication and errors in Breeze" - I have a task for it in https://github.com/apache/airflow/issues/22906
--
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 #23189: Fix and improve consistency of checking command return code
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #23189:
URL: https://github.com/apache/airflow/pull/23189#issuecomment-1107529161
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, 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