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

Improve error handling in cli and introduce consistency #12764

Merged
merged 4 commits into from
Dec 4, 2020

Conversation

turbaszek
Copy link
Member

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 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.

This PR is a followup after apache#12375 and apache#12704 it improves handling
of some errors in cli commands to avoid show users to much traceback
and uses SystemExit consitently.
@boring-cyborg boring-cyborg bot added area:CLI provider:cncf-kubernetes Kubernetes provider related issues labels Dec 2, 2020
if uri_parts.scheme == '' or uri_parts.netloc == '':
return False
return True
return uri_parts.scheme != '' or uri_parts.netloc != ''
Copy link
Member

Choose a reason for hiding this comment

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

This should be changed into uri_parts.scheme != '' and uri_parts.netloc != '', instinct from a math-major guy ;-)

Copy link
Member Author

@turbaszek turbaszek Dec 2, 2020

Choose a reason for hiding this comment

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

Right, De Morgan's laws - and I did a thesis from fuzzy logic 🤦

Copy link
Member

Choose a reason for hiding this comment

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

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😄

Copy link
Member

@XD-DENG XD-DENG left a comment

Choose a reason for hiding this comment

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

Other than the minor one in my comment below, LGTM

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

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@turbaszek turbaszek Dec 2, 2020

Choose a reason for hiding this comment

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

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.

@github-actions
Copy link

github-actions bot commented Dec 2, 2020

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.

@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 Dec 2, 2020
@turbaszek
Copy link
Member Author

@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

@XD-DENG
Copy link
Member

XD-DENG commented Dec 4, 2020

I'm good with it 👍

@turbaszek turbaszek merged commit 1bd98cd into apache:master Dec 4, 2020
@turbaszek turbaszek deleted the cli-consistency branch December 4, 2020 09:41
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 provider:cncf-kubernetes Kubernetes provider related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants