Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return nonzero exit codes on pool import errors. #12095

Merged
merged 1 commit into from
Nov 22, 2020

Conversation

jmcarp
Copy link
Contributor

@jmcarp jmcarp commented Nov 4, 2020

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 for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@github-actions
Copy link

github-actions bot commented Nov 4, 2020

The Workflow run 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*.

@github-actions
Copy link

github-actions bot commented Nov 4, 2020

The Workflow run 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*.

@jmcarp jmcarp force-pushed the jmcarp/pool-command-exit-status branch from c7cbd83 to c676520 Compare November 4, 2020 20:08
@jmcarp jmcarp marked this pull request as ready for review November 4, 2020 22:44
@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Nov 5, 2020
@github-actions
Copy link

github-actions bot commented Nov 5, 2020

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!

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)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use f-strings?

else:
failed.append(k)
print("{} of {} pool(s) successfully updated.".format(len(pools), len(pools_json)))
return pools, failed # pylint: disable=lost-exception
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return pools, failed # pylint: disable=lost-exception
return pools, failed

Should we be able to remove it?

@jmcarp jmcarp force-pushed the jmcarp/pool-command-exit-status branch from c676520 to 895ddee Compare November 8, 2020 20:14
@jmcarp
Copy link
Contributor Author

jmcarp commented Nov 8, 2020

Thanks for reviewing @turbaszek, I updated the branch.

@jmcarp jmcarp force-pushed the jmcarp/pool-command-exit-status branch from 895ddee to 70ba350 Compare November 9, 2020 15:07
@github-actions
Copy link

github-actions bot commented Nov 9, 2020

The Workflow run 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*.

@jmcarp jmcarp force-pushed the jmcarp/pool-command-exit-status branch from 70ba350 to ed03cb9 Compare November 10, 2020 04:42
@jmcarp
Copy link
Contributor Author

jmcarp commented Nov 11, 2020

@kaxil @turbaszek is it all right that some "quarantined" tests are failing? I don't think they're related to these changes.

@potiuk
Copy link
Member

potiuk commented Nov 11, 2020

@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: #10118 and Quarantine Issues that are occasionally failing and are quarantined

BTW. If you would like to help with that - feel free to pick any of those issues :).

@jmcarp jmcarp force-pushed the jmcarp/pool-command-exit-status branch from ed03cb9 to 9ac753e Compare November 18, 2020 16:49
@github-actions
Copy link

The Workflow run 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*.

@jmcarp jmcarp force-pushed the jmcarp/pool-command-exit-status branch from 9ac753e to e7cc6f9 Compare November 19, 2020 02:38
@jmcarp
Copy link
Contributor Author

jmcarp commented Nov 19, 2020

Not sure what changed, but I rebased on master, and tests are passing now. Is this ok to merge?

Comment on lines 73 to 74
print("Failed to update pool(s): {}".format(", ".join(failed)))
sys.exit(1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print("Failed to update pool(s): {}".format(", ".join(failed)))
sys.exit(1)
exit("Failed to update pool(s): {}".format(", ".join(failed)))

This way the error goes to stderr, not stdout

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that we use a mixture of sys.exit and raise SystemExit across the cli module. Do we prefer one or the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I picked sys.exit arbitrarily for now. We can pick one invocation and use it consistently throughout the module separately.

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small change please

@github-actions github-actions bot removed the okay to merge It's ok to merge this PR as it does not require more tests label Nov 19, 2020
@ashb ashb added this to the Airflow 2.0.0-beta4 milestone Nov 19, 2020
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.
@jmcarp jmcarp force-pushed the jmcarp/pool-command-exit-status branch from e7cc6f9 to d485cfc Compare November 21, 2020 20:12
@github-actions
Copy link

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!

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Nov 21, 2020
@jmcarp jmcarp merged commit 397d912 into apache:master Nov 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CLI okay to merge It's ok to merge this PR as it does not require more tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants