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/12/02 20:17:36 UTC
[GitHub] [airflow] turbaszek opened a new pull request #12764: Improve error handling in cli and introduce consistency
turbaszek opened a new pull request #12764:
URL: https://github.com/apache/airflow/pull/12764
This PR is a followup after #12375 and #12704 it improves handling
of some errors in cli commands to avoid show users to much traceback
and uses SystemExit consistently.
---
**^ 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] turbaszek commented on pull request #12764: Improve error handling in cli and introduce consistency
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #12764:
URL: https://github.com/apache/airflow/pull/12764#issuecomment-738666129
@kaxil @XD-DENG should we be good to merge this PR? There's a failure in quarantine tests and postgres was killed with 137 error
----------------------------------------------------------------
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] turbaszek commented on a change in pull request #12764: Improve error handling in cli and introduce consistency
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12764:
URL: https://github.com/apache/airflow/pull/12764#discussion_r535120274
##########
File path: airflow/cli/commands/connection_command.py
##########
@@ -140,20 +136,19 @@ def connections_export(args):
_, filetype = os.path.splitext(args.file.name)
filetype = filetype.lower()
if filetype not in allowed_formats:
- msg = (
- f"Unsupported file format. "
- f"The file must have the extension {', '.join(allowed_formats)}"
+ raise SystemExit(
+ f"Unsupported file format. The file must have "
+ f"the extension {', '.join(allowed_formats)}."
)
- raise SystemExit(msg)
connections = session.query(Connection).order_by(Connection.conn_id).all()
msg = _format_connections(connections, filetype)
args.file.write(msg)
if _is_stdout(args.file):
- print("Connections successfully exported.", file=sys.stderr)
+ print(f"Connections successfully exported to {args.file.name}.")
else:
- print(f"Connections successfully exported to {args.file.name}")
+ print("Connections successfully exported.", file=sys.stderr)
Review comment:
We should probably unify this method to work with the `output` argument as in #12704, this will introduce more consistency. I will do this in next PR
----------------------------------------------------------------
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] turbaszek commented on a change in pull request #12764: Improve error handling in cli and introduce consistency
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12764:
URL: https://github.com/apache/airflow/pull/12764#discussion_r534519083
##########
File path: airflow/cli/commands/connection_command.py
##########
@@ -112,17 +112,13 @@ def _format_connections(conns: List[Connection], fmt: str) -> str:
def _is_stdout(fileio: io.TextIOWrapper) -> bool:
- if fileio.name == '<stdout>':
- return True
- return False
+ return fileio.name == '<stdout>'
def _valid_uri(uri: str) -> bool:
"""Check if a URI is valid, by checking if both scheme and netloc are available"""
uri_parts = urlparse(uri)
- if uri_parts.scheme == '' or uri_parts.netloc == '':
- return False
- return True
+ return uri_parts.scheme != '' or uri_parts.netloc != ''
Review comment:
Right, De Morgans laws - and I did a thesis from fuzzy logic 🤦
##########
File path: airflow/cli/commands/connection_command.py
##########
@@ -112,17 +112,13 @@ def _format_connections(conns: List[Connection], fmt: str) -> str:
def _is_stdout(fileio: io.TextIOWrapper) -> bool:
- if fileio.name == '<stdout>':
- return True
- return False
+ return fileio.name == '<stdout>'
def _valid_uri(uri: str) -> bool:
"""Check if a URI is valid, by checking if both scheme and netloc are available"""
uri_parts = urlparse(uri)
- if uri_parts.scheme == '' or uri_parts.netloc == '':
- return False
- return True
+ return uri_parts.scheme != '' or uri_parts.netloc != ''
Review comment:
Right, De Morgan's laws - and I did a thesis from fuzzy logic 🤦
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #12764: Improve error handling in cli and introduce consistency
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12764:
URL: https://github.com/apache/airflow/pull/12764#discussion_r535097197
##########
File path: airflow/cli/commands/connection_command.py
##########
@@ -140,20 +136,19 @@ def connections_export(args):
_, filetype = os.path.splitext(args.file.name)
filetype = filetype.lower()
if filetype not in allowed_formats:
- msg = (
- f"Unsupported file format. "
- f"The file must have the extension {', '.join(allowed_formats)}"
+ raise SystemExit(
+ f"Unsupported file format. The file must have "
+ f"the extension {', '.join(allowed_formats)}."
)
- raise SystemExit(msg)
connections = session.query(Connection).order_by(Connection.conn_id).all()
msg = _format_connections(connections, filetype)
args.file.write(msg)
if _is_stdout(args.file):
- print("Connections successfully exported.", file=sys.stderr)
+ print(f"Connections successfully exported to {args.file.name}.")
else:
- print(f"Connections successfully exported to {args.file.name}")
+ print("Connections successfully exported.", file=sys.stderr)
Review comment:
It works fine on the master branch
----------------------------------------------------------------
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] turbaszek commented on a change in pull request #12764: Improve error handling in cli and introduce consistency
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12764:
URL: https://github.com/apache/airflow/pull/12764#discussion_r534563911
##########
File path: airflow/cli/commands/variable_command.py
##########
@@ -95,7 +96,7 @@ def _import_helper(filepath):
fail_count += 1
else:
suc_count += 1
- print("{} of {} variables successfully updated.".format(suc_count, len(var_json)))
+ print(f"{suc_count} of {len(var_json)} variables successfully updated.")
Review comment:
I tried, but then I resigned because I'm not sure what's better:
- `some text and 6` or `some text and 6.`
- `some 'text'=value` or `some 'text'=value.`
- 'some /text/and/path` or `some /text/and/path.`
----------------------------------------------------------------
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] turbaszek commented on a change in pull request #12764: Improve error handling in cli and introduce consistency
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12764:
URL: https://github.com/apache/airflow/pull/12764#discussion_r534563911
##########
File path: airflow/cli/commands/variable_command.py
##########
@@ -95,7 +96,7 @@ def _import_helper(filepath):
fail_count += 1
else:
suc_count += 1
- print("{} of {} variables successfully updated.".format(suc_count, len(var_json)))
+ print(f"{suc_count} of {len(var_json)} variables successfully updated.")
Review comment:
I tried, but then I resigned because I'm not sure what's better:
- `some text and 6` or `some text and 6.`
- `some 'text'=value` or `some 'text'=value.`
- `some /text/and/path` or `some /text/and/path.`
----------------------------------------------------------------
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] XD-DENG commented on a change in pull request #12764: Improve error handling in cli and introduce consistency
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #12764:
URL: https://github.com/apache/airflow/pull/12764#discussion_r534531538
##########
File path: airflow/cli/commands/variable_command.py
##########
@@ -95,7 +96,7 @@ def _import_helper(filepath):
fail_count += 1
else:
suc_count += 1
- print("{} of {} variables successfully updated.".format(suc_count, len(var_json)))
+ print(f"{suc_count} of {len(var_json)} variables successfully updated.")
Review comment:
From other changes I had the impression that "removing following period" is one of the the points you are enforcing in this PR. Maybe you missed this one?
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #12764: Improve error handling in cli and introduce consistency
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12764:
URL: https://github.com/apache/airflow/pull/12764#discussion_r535088301
##########
File path: airflow/cli/commands/connection_command.py
##########
@@ -140,20 +136,19 @@ def connections_export(args):
_, filetype = os.path.splitext(args.file.name)
filetype = filetype.lower()
if filetype not in allowed_formats:
- msg = (
- f"Unsupported file format. "
- f"The file must have the extension {', '.join(allowed_formats)}"
+ raise SystemExit(
+ f"Unsupported file format. The file must have "
+ f"the extension {', '.join(allowed_formats)}."
)
- raise SystemExit(msg)
connections = session.query(Connection).order_by(Connection.conn_id).all()
msg = _format_connections(connections, filetype)
args.file.write(msg)
if _is_stdout(args.file):
- print("Connections successfully exported.", file=sys.stderr)
+ print(f"Connections successfully exported to {args.file.name}.")
else:
- print(f"Connections successfully exported to {args.file.name}")
+ print("Connections successfully exported.", file=sys.stderr)
Review comment:
If we display the output on stdout, we cannot write another message to that stream.
----------------------------------------------------------------
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] XD-DENG commented on a change in pull request #12764: Improve error handling in cli and introduce consistency
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #12764:
URL: https://github.com/apache/airflow/pull/12764#discussion_r534522391
##########
File path: airflow/cli/commands/connection_command.py
##########
@@ -112,17 +112,13 @@ def _format_connections(conns: List[Connection], fmt: str) -> str:
def _is_stdout(fileio: io.TextIOWrapper) -> bool:
- if fileio.name == '<stdout>':
- return True
- return False
+ return fileio.name == '<stdout>'
def _valid_uri(uri: str) -> bool:
"""Check if a URI is valid, by checking if both scheme and netloc are available"""
uri_parts = urlparse(uri)
- if uri_parts.scheme == '' or uri_parts.netloc == '':
- return False
- return True
+ return uri_parts.scheme != '' or uri_parts.netloc != ''
Review comment:
Ha, only now I know you also majored in math😄 My focus was random process and application in bio-stats.
But anyway, not rare for math people like us to miss OR/AND time to time😄
----------------------------------------------------------------
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] turbaszek merged pull request #12764: Improve error handling in cli and introduce consistency
Posted by GitBox <gi...@apache.org>.
turbaszek merged pull request #12764:
URL: https://github.com/apache/airflow/pull/12764
----------------------------------------------------------------
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] XD-DENG commented on a change in pull request #12764: Improve error handling in cli and introduce consistency
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #12764:
URL: https://github.com/apache/airflow/pull/12764#discussion_r534456055
##########
File path: airflow/cli/commands/connection_command.py
##########
@@ -112,17 +112,13 @@ def _format_connections(conns: List[Connection], fmt: str) -> str:
def _is_stdout(fileio: io.TextIOWrapper) -> bool:
- if fileio.name == '<stdout>':
- return True
- return False
+ return fileio.name == '<stdout>'
def _valid_uri(uri: str) -> bool:
"""Check if a URI is valid, by checking if both scheme and netloc are available"""
uri_parts = urlparse(uri)
- if uri_parts.scheme == '' or uri_parts.netloc == '':
- return False
- return True
+ return uri_parts.scheme != '' or uri_parts.netloc != ''
Review comment:
This should be changed into `uri_parts.scheme != '' and uri_parts.netloc != ''`, instinct from a math-major guy ;-)
----------------------------------------------------------------
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] XD-DENG commented on pull request #12764: Improve error handling in cli and introduce consistency
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #12764:
URL: https://github.com/apache/airflow/pull/12764#issuecomment-738666934
I'm good with 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] turbaszek commented on a change in pull request #12764: Improve error handling in cli and introduce consistency
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12764:
URL: https://github.com/apache/airflow/pull/12764#discussion_r535022600
##########
File path: airflow/cli/commands/connection_command.py
##########
@@ -140,20 +136,19 @@ def connections_export(args):
_, filetype = os.path.splitext(args.file.name)
filetype = filetype.lower()
if filetype not in allowed_formats:
- msg = (
- f"Unsupported file format. "
- f"The file must have the extension {', '.join(allowed_formats)}"
+ raise SystemExit(
+ f"Unsupported file format. The file must have "
+ f"the extension {', '.join(allowed_formats)}."
)
- raise SystemExit(msg)
connections = session.query(Connection).order_by(Connection.conn_id).all()
msg = _format_connections(connections, filetype)
args.file.write(msg)
if _is_stdout(args.file):
- print("Connections successfully exported.", file=sys.stderr)
+ print(f"Connections successfully exported to {args.file.name}.")
else:
- print(f"Connections successfully exported to {args.file.name}")
+ print("Connections successfully exported.", file=sys.stderr)
Review comment:
I think this changes itroduces expected behaviour. If the file is `stdout` then print to `stdout` ot `stderr`. But I can be wrong, I think this was done by @mik-laj
----------------------------------------------------------------
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 #12764: Improve error handling in cli and introduce consistency
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12764:
URL: https://github.com/apache/airflow/pull/12764#discussion_r534566513
##########
File path: airflow/cli/commands/connection_command.py
##########
@@ -140,20 +136,19 @@ def connections_export(args):
_, filetype = os.path.splitext(args.file.name)
filetype = filetype.lower()
if filetype not in allowed_formats:
- msg = (
- f"Unsupported file format. "
- f"The file must have the extension {', '.join(allowed_formats)}"
+ raise SystemExit(
+ f"Unsupported file format. The file must have "
+ f"the extension {', '.join(allowed_formats)}."
)
- raise SystemExit(msg)
connections = session.query(Connection).order_by(Connection.conn_id).all()
msg = _format_connections(connections, filetype)
args.file.write(msg)
if _is_stdout(args.file):
- print("Connections successfully exported.", file=sys.stderr)
+ print(f"Connections successfully exported to {args.file.name}.")
else:
- print(f"Connections successfully exported to {args.file.name}")
+ print("Connections successfully exported.", file=sys.stderr)
Review comment:
Can you explain this change please
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #12764: Improve error handling in cli and introduce consistency
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12764:
URL: https://github.com/apache/airflow/pull/12764#discussion_r535096096
##########
File path: airflow/cli/commands/connection_command.py
##########
@@ -140,20 +136,19 @@ def connections_export(args):
_, filetype = os.path.splitext(args.file.name)
filetype = filetype.lower()
if filetype not in allowed_formats:
- msg = (
- f"Unsupported file format. "
- f"The file must have the extension {', '.join(allowed_formats)}"
+ raise SystemExit(
+ f"Unsupported file format. The file must have "
+ f"the extension {', '.join(allowed_formats)}."
)
- raise SystemExit(msg)
connections = session.query(Connection).order_by(Connection.conn_id).all()
msg = _format_connections(connections, filetype)
args.file.write(msg)
if _is_stdout(args.file):
- print("Connections successfully exported.", file=sys.stderr)
+ print(f"Connections successfully exported to {args.file.name}.")
else:
- print(f"Connections successfully exported to {args.file.name}")
+ print("Connections successfully exported.", file=sys.stderr)
Review comment:
Before:
```
$ airflow connections export - | jq .
....
"yandexcloud_default": {
"conn_type": "yandexcloud",
"description": null,
"host": null,
"login": null,
"password": null,
"schema": "default",
"port": null,
"extra": null
}
}
```
After
```
$ airflow connections export - | jq .
.....
"login": null,
"password": null,
"schema": "default",
"port": null,
"extra": null
}
}
parse error: Invalid numeric literal at line 472, column 13
```
----------------------------------------------------------------
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 #12764: Improve error handling in cli and introduce consistency
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12764:
URL: https://github.com/apache/airflow/pull/12764#issuecomment-737540735
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