-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Refactor list rendering in commands #12704
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
Refactor list rendering in commands #12704
Conversation
|
The only thing that I'm wondering about is suppressing / removing log records when using json/yaml. Because otherwise the output is not valid. For example: or It can also appear inline with the output: I've never seen this in any other application, so I lean toward suppressing warnings and logs. |
The same preference for me. For |
XD-DENG
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
780d3da to
02a5ff2
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
02a5ff2 to
48416ed
Compare
UPDATING.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
airflow pools import and airflow providers hooks should be in this list as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in caf81ff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@turbaszek after your latest change (some commands don't print table anymore), maybe we need to further update this doc as well.
airflow/cli/commands/pool_command.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just print "Pool deleted" instead of printing info about it?
airflow/cli/commands/pool_command.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, most of clis I know would show "Pool created" instead of showing info. We can consider a verbose flag.
airflow/cli/commands/pool_command.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are exporting pools to file, I think we shouldn't be printing them after that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I agree with the three points you raised (including this one).
airflow/cli/commands/pool_command.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add try-except similar to what you added in pool_delete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to address this in followup PR. Probably create new decorator that will handle known errors and show only message instead of lines of traceback to make it more user friendly.
922abe5 to
87533da
Compare
|
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*. |
|
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*. |
This commit unifies the mechanism of rendering output of tabular data. This gives users a possibility to eiter display a tabular representation of data or render it as valid json or yaml payload. Closes: apache#12699
87533da to
0006363
Compare
XD-DENG
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe other folks will have further suggestions. From my point of view, all my questions/concerns have been addressed well.
Nice feature indeed. Thanks @turbaszek
|
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 master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
0006363 to
d12f31a
Compare
Co-authored-by: Kaxil Naik <kaxilnaik@gmail.com>
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.
This commit unifies the mechanism of rendering output of tabular
data. This gives users a possibility to either display a tabular
representation of data or render it as valid json or yaml payload.
Closes: #12699
Instead of using tabulate to render commands output we use rich. Due to this change the
--outputargument will no longer accept formats of tabulate tables. Instead it accepts:table- will render the output in predefined tablejson- will render the output as a jsonyaml- will render the output as yamlBy doing this we increased consistency and gave users possibility to manipulate the output programmatically (when using json or yaml).
Affected commands:
airflow dags listairflow dags reportairflow dags list-runsairflow dags list-jobsairflow connections listairflow connections getairflow pools listairflow pools getairflow pools setairflow pools deleteairflow pools exportairflow role listairflow providers listairflow providers getairflow tasks states-for-dag-runairflow users listairflow variables listExample:
Table:
JSON:
YAML:
Using
jqUsing
yq^ 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.