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/11/04 19:07:54 UTC
[GitHub] [airflow] jmcarp opened a new pull request #12095: Return nonzero exit codes on pool import errors.
jmcarp opened a new pull request #12095:
URL: https://github.com/apache/airflow/pull/12095
The pool import command returns an exit code of zero in a few different
error cases. This causes problems for scripts that invoke the command,
since commands that actually failed will appear to have worked. This
patch returns a nonzero code if the pool file doesn't exist, if the file
isn't valid json, or if any of the pools in the file is invalid.
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] jmcarp commented on pull request #12095: Return nonzero exit codes on pool import errors.
Posted by GitBox <gi...@apache.org>.
jmcarp commented on pull request #12095:
URL: https://github.com/apache/airflow/pull/12095#issuecomment-725592527
@kaxil @turbaszek is it all right that some "quarantined" tests are failing? I don't think they're related to these 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jmcarp commented on a change in pull request #12095: Return nonzero exit codes on pool import errors.
Posted by GitBox <gi...@apache.org>.
jmcarp commented on a change in pull request #12095:
URL: https://github.com/apache/airflow/pull/12095#discussion_r528237803
##########
File path: airflow/cli/commands/pool_command.py
##########
@@ -63,13 +64,14 @@ def pool_delete(args):
@cli_utils.action_logging
def pool_import(args):
"""Imports pools from the file"""
- api_client = get_current_api_client()
- if os.path.exists(args.file):
- pools = pool_import_helper(args.file)
- else:
+ if not os.path.exists(args.file):
print("Missing pools file.")
- pools = api_client.get_pools()
+ sys.exit(1)
+ pools, failed = pool_import_helper(args.file)
print(_tabulate_pools(pools=pools, tablefmt=args.output))
+ if len(failed) > 0:
+ print("Failed to update pool(s): {}".format(", ".join(failed)))
+ sys.exit(1)
Review comment:
I picked `sys.exit` arbitrarily for now. We can pick one invocation and use it consistently throughout the module separately.
----------------------------------------------------------------
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 #12095: Return nonzero exit codes on pool import errors.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12095:
URL: https://github.com/apache/airflow/pull/12095#issuecomment-724082499
[The Workflow run](https://github.com/apache/airflow/actions/runs/354190198) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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 #12095: Return nonzero exit codes on pool import errors.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12095:
URL: https://github.com/apache/airflow/pull/12095#issuecomment-721931952
[The Workflow run](https://github.com/apache/airflow/actions/runs/346181631) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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] ashb commented on a change in pull request #12095: Return nonzero exit codes on pool import errors.
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12095:
URL: https://github.com/apache/airflow/pull/12095#discussion_r526967986
##########
File path: airflow/cli/commands/pool_command.py
##########
@@ -63,13 +64,14 @@ def pool_delete(args):
@cli_utils.action_logging
def pool_import(args):
"""Imports pools from the file"""
- api_client = get_current_api_client()
- if os.path.exists(args.file):
- pools = pool_import_helper(args.file)
- else:
+ if not os.path.exists(args.file):
print("Missing pools file.")
- pools = api_client.get_pools()
+ sys.exit(1)
+ pools, failed = pool_import_helper(args.file)
print(_tabulate_pools(pools=pools, tablefmt=args.output))
+ if len(failed) > 0:
+ print("Failed to update pool(s): {}".format(", ".join(failed)))
+ sys.exit(1)
Review comment:
```suggestion
exit("Failed to update pool(s): {}".format(", ".join(failed)))
```
This way the error goes to stderr, not stdout
----------------------------------------------------------------
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] jmcarp commented on a change in pull request #12095: Return nonzero exit codes on pool import errors.
Posted by GitBox <gi...@apache.org>.
jmcarp commented on a change in pull request #12095:
URL: https://github.com/apache/airflow/pull/12095#discussion_r527021286
##########
File path: airflow/cli/commands/pool_command.py
##########
@@ -63,13 +64,14 @@ def pool_delete(args):
@cli_utils.action_logging
def pool_import(args):
"""Imports pools from the file"""
- api_client = get_current_api_client()
- if os.path.exists(args.file):
- pools = pool_import_helper(args.file)
- else:
+ if not os.path.exists(args.file):
print("Missing pools file.")
- pools = api_client.get_pools()
+ sys.exit(1)
+ pools, failed = pool_import_helper(args.file)
print(_tabulate_pools(pools=pools, tablefmt=args.output))
+ if len(failed) > 0:
+ print("Failed to update pool(s): {}".format(", ".join(failed)))
+ sys.exit(1)
Review comment:
I see that we use a mixture of `sys.exit` and `raise SystemExit` across the cli module. Do we prefer one or the other?
----------------------------------------------------------------
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 #12095: Return nonzero exit codes on pool import errors.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12095:
URL: https://github.com/apache/airflow/pull/12095#discussion_r517935841
##########
File path: airflow/cli/commands/pool_command.py
##########
@@ -87,24 +89,17 @@ def pool_import_helper(filepath):
try: # pylint: disable=too-many-nested-blocks
pools_json = json.loads(data)
except JSONDecodeError as e:
- print("Please check the validity of the json file: " + str(e))
- else:
- try:
- pools = []
- counter = 0
- for k, v in pools_json.items():
- if isinstance(v, dict) and len(v) == 2:
- pools.append(
- api_client.create_pool(name=k, slots=v["slots"], description=v["description"])
- )
- counter += 1
- else:
- pass
- except Exception: # pylint: disable=broad-except
- pass
- finally:
- print("{} of {} pool(s) successfully updated.".format(counter, len(pools_json)))
- return pools # pylint: disable=lost-exception
+ print("Invalid json file: " + str(e))
+ sys.exit(1)
+ pools = []
+ failed = []
+ for k, v in pools_json.items():
+ if isinstance(v, dict) and len(v) == 2:
+ pools.append(api_client.create_pool(name=k, slots=v["slots"], description=v["description"]))
+ else:
+ failed.append(k)
+ print("{} of {} pool(s) successfully updated.".format(len(pools), len(pools_json)))
+ return pools, failed # pylint: disable=lost-exception
Review comment:
```suggestion
return pools, failed
```
Should we be able to remove 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] jmcarp merged pull request #12095: Return nonzero exit codes on pool import errors.
Posted by GitBox <gi...@apache.org>.
jmcarp merged pull request #12095:
URL: https://github.com/apache/airflow/pull/12095
----------------------------------------------------------------
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 #12095: Return nonzero exit codes on pool import errors.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12095:
URL: https://github.com/apache/airflow/pull/12095#issuecomment-722262502
The PR should be OK to be merged with just subset of tests as it does not modify Core of Airflow. The committers might merge it or can add a label 'full tests needed' and re-run it to run all tests if they see it is needed!
----------------------------------------------------------------
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] jmcarp commented on pull request #12095: Return nonzero exit codes on pool import errors.
Posted by GitBox <gi...@apache.org>.
jmcarp commented on pull request #12095:
URL: https://github.com/apache/airflow/pull/12095#issuecomment-730438420
Not sure what changed, but I rebased on master, and tests are passing now. Is this ok to merge?
----------------------------------------------------------------
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] jmcarp commented on pull request #12095: Return nonzero exit codes on pool import errors.
Posted by GitBox <gi...@apache.org>.
jmcarp commented on pull request #12095:
URL: https://github.com/apache/airflow/pull/12095#issuecomment-723659867
Thanks for reviewing @turbaszek, I updated the 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] potiuk commented on pull request #12095: Return nonzero exit codes on pool import errors.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12095:
URL: https://github.com/apache/airflow/pull/12095#issuecomment-725594573
@jmcarp Some of the quarantined tests started to fail recently and we have an on-going effort to fix them before 2.0 is out. See the automated issue here: https://github.com/apache/airflow/issues/10118 and https://github.com/apache/airflow/labels/Quarantine
BTW. If you would like to help with that - feel free to pick any of those issues :).
----------------------------------------------------------------
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 #12095: Return nonzero exit codes on pool import errors.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12095:
URL: https://github.com/apache/airflow/pull/12095#issuecomment-731641007
The PR should be OK to be merged with just subset of tests as it does not modify Core of Airflow. The committers might merge it or can add a label 'full tests needed' and re-run it to run all tests if they see it is needed!
----------------------------------------------------------------
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 #12095: Return nonzero exit codes on pool import errors.
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12095:
URL: https://github.com/apache/airflow/pull/12095#discussion_r517935602
##########
File path: airflow/cli/commands/pool_command.py
##########
@@ -87,24 +89,17 @@ def pool_import_helper(filepath):
try: # pylint: disable=too-many-nested-blocks
pools_json = json.loads(data)
except JSONDecodeError as e:
- print("Please check the validity of the json file: " + str(e))
- else:
- try:
- pools = []
- counter = 0
- for k, v in pools_json.items():
- if isinstance(v, dict) and len(v) == 2:
- pools.append(
- api_client.create_pool(name=k, slots=v["slots"], description=v["description"])
- )
- counter += 1
- else:
- pass
- except Exception: # pylint: disable=broad-except
- pass
- finally:
- print("{} of {} pool(s) successfully updated.".format(counter, len(pools_json)))
- return pools # pylint: disable=lost-exception
+ print("Invalid json file: " + str(e))
+ sys.exit(1)
+ pools = []
+ failed = []
+ for k, v in pools_json.items():
+ if isinstance(v, dict) and len(v) == 2:
+ pools.append(api_client.create_pool(name=k, slots=v["slots"], description=v["description"]))
+ else:
+ failed.append(k)
+ print("{} of {} pool(s) successfully updated.".format(len(pools), len(pools_json)))
Review comment:
Should we use f-strings?
----------------------------------------------------------------
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 #12095: Return nonzero exit codes on pool import errors.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12095:
URL: https://github.com/apache/airflow/pull/12095#issuecomment-729819416
[The Workflow run](https://github.com/apache/airflow/actions/runs/370670233) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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 #12095: Return nonzero exit codes on pool import errors.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12095:
URL: https://github.com/apache/airflow/pull/12095#issuecomment-721931829
[The Workflow run](https://github.com/apache/airflow/actions/runs/346180998) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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