Skip to content

Use rich to render info and cheat-sheet command#12689

Merged
turbaszek merged 7 commits intoapache:masterfrom
PolideaInternal:improve-info-ux
Nov 28, 2020
Merged

Use rich to render info and cheat-sheet command#12689
turbaszek merged 7 commits intoapache:masterfrom
PolideaInternal:improve-info-ux

Conversation

@turbaszek
Copy link
Member

@turbaszek turbaszek commented Nov 28, 2020

airflow info

Before:
Screenshot 2020-11-28 at 16 42 32

After:
Screenshot 2020-11-28 at 16 41 34

airflow cheat-sheet

Before:
Screenshot 2020-11-28 at 19 07 52

After:
Screenshot 2020-11-28 at 19 46 18


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

@mik-laj
Copy link
Member

mik-laj commented Nov 28, 2020

This documentation contains sample output from this command. Can you update it?
http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/apache-airflow/latest/modules_management.html

@turbaszek
Copy link
Member Author

http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/apache-airflow/latest/modules_management.html

Sure, is it only for me or the images are not rendering on this page?

@mik-laj

This comment has been minimized.

@turbaszek

This comment has been minimized.

@mik-laj

This comment has been minimized.

@turbaszek turbaszek marked this pull request as ready for review November 28, 2020 17:47
print("Report uploaded.")
print()
print("Link:\t", link)
print(link)
Copy link
Member Author

@turbaszek turbaszek Nov 28, 2020

Choose a reason for hiding this comment

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

This simplifies output so users can do airflow info --file-io | grep https | pbcopy. Example:

Uploading report to file.io service.
Report uploaded.
https://file.io/xfDPsSSxkFm9

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

@turbaszek turbaszek changed the title Use rich to render airflow info command Use rich to render info and cheat-sheet command Nov 28, 2020
@turbaszek turbaszek requested review from XD-DENG and potiuk November 28, 2020 18:56
@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*.

Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

❤️

@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 28, 2020
@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!

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

❤️

Comment on lines 36 to 37
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 fix CI failure

Suggested change
prefix: List[str], commands: Iterable[Union[GroupCommand, ActionCommand]],
help_msg: Optional[str] = None
prefix: List[str],
commands: Iterable[Union[GroupCommand, ActionCommand]],
help_msg: Optional[str] = None

Copy link
Member Author

@turbaszek turbaszek Nov 28, 2020

Choose a reason for hiding this comment

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

Yeah, thanks! I fixed it locally but did not add this to commit 🤦

Config info
executor | LocalExecutor
task_logging_handler | airflow.utils.log.file_task_handler.FileTaskHandler
sql_alchemy_conn | postgresql+psycopg2://postgres:airflow@postgres/airflow
Copy link
Member

@XD-DENG XD-DENG Nov 28, 2020

Choose a reason for hiding this comment

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

If everything is working correctly, the sample value here should be something like postgresql+psycopg2://p...s:PASSWORD@postgres/airflow instead, according to

def process_url(self, value):

May you please help double check?

OR, personally I think we can consider removing sql_alchemy_conn from the info command output

ADD-UP: if we show it here, either we are showing something useful BUT sensitive, or we are showing something not very useful (after sufficient necessary masking), IMHO

Copy link
Member Author

Choose a reason for hiding this comment

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

In my opinion it works as expected. I can use example generated with --anonymize flag:

root@1aa14208df15:/opt/airflow# airflow info --anonymize
...
Config info
executor             | LocalExecutor
task_logging_handler | airflow.utils.log.file_task_handler.FileTaskHandler
sql_alchemy_conn     | postgresql+psycopg2://p...s:PASSWORD@postgres/airflow

If some one can run airflow info in your deployment - it's too late, so masking information like conn uri (which is also accessible via airflow config command) will not increase security.

Copy link
Member

Choose a reason for hiding this comment

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

My bad that I missed the --anonymize flag.

Regarding whether to keep sql_alchemy_conn in the output of airflow info, I still keep my view given it's (almost) the only sensitive item in the output. But I will not insist in this opinion in this PR.

OR, at least, let's make anonymisation default. Currently it's store_true, i.e. no anonymisation by default. Let me know your thoughts?

ARG_ANONYMIZE = Arg(
('--anonymize',),
help='Minimize any personal identifiable information. Use it when sharing output with others.',
action='store_true',
)

Copy link
Member

Choose a reason for hiding this comment

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

But can be discussed & addressed in separate PR anyway, if you prefer. I will approve this PR first.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created a separate issue: #12696

@turbaszek turbaszek requested a review from XD-DENG November 28, 2020 21:49
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

Comments