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

Add 'Show record' option for variables #21342

Merged
merged 12 commits into from
Feb 28, 2022
Merged

Conversation

aa3pankaj
Copy link
Contributor

@aa3pankaj aa3pankaj commented Feb 5, 2022

Show record option in "List Variable" table:

Screenshot 2022-02-05 at 8 58 07 PM

"Show Variable" page:

Screenshot 2022-02-08 at 5 23 39 AM

"Edit Variable" page (Existing feature, adding screenshot below to show the difference in formatting compared to "Show variable" page):

Screenshot 2022-02-08 at 5 23 57 AM

closes: #18436

@boring-cyborg boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Feb 5, 2022
@aa3pankaj aa3pankaj marked this pull request as ready for review February 5, 2022 15:40
@aa3pankaj aa3pankaj changed the title Add show option for variables Add 'Show record' option for variables Feb 5, 2022
@SangwanP
Copy link
Contributor

SangwanP commented Feb 5, 2022

Hi @aa3pankaj, thanks for working on this issue, I had initially opened but could never get to working on it.
I just want to confirm one thing — is the formatting of the variable preserved when viewing it through the ‘show view’?

@aa3pankaj
Copy link
Contributor Author

@SangwanP added screenshot of "Edit Variable" page, there is some difference in formatting but this is better than what we have in the "List Variable"

airflow/www/widgets.py Outdated Show resolved Hide resolved
@SangwanP
Copy link
Contributor

SangwanP commented Feb 7, 2022

@SangwanP added screenshot of "Edit Variable" page, there is some difference in formatting but this is better than what we have in the "List Variable"

@aa3pankaj -- Is it possible to preserve the formatting while displaying the Variable through the 'Show' page, like it is preserved in the 'Edit' page? As I mentioned in the issue: #18436, the goal is that the Variable can be viewed as-is through this Show page, preserving the formatting, and not having to pass through a json formatter to get the correct indentation.

@ashb
Copy link
Member

ashb commented Feb 7, 2022

white-space: pre-wrap; is the CSS style you are looking for I suspect. Make sure it only gets applied to Variables though, not all Show pages.

@aa3pankaj aa3pankaj requested a review from ashb February 7, 2022 23:56
airflow/www/views.py Outdated Show resolved Hide resolved
@ashb
Copy link
Member

ashb commented Feb 8, 2022

Looking at this a bit more, this possibly poses a security risk. Right now "sensitive" values are masked (i.e. not shown in the UI) -- try this and create a Variable called "api_key" and you'll see it as *******, but if we add a show page it won't mask it, making the value readable by anyone with read access to Variables which is likely not what we want.

@potiuk
Copy link
Member

potiuk commented Feb 15, 2022

Yeah. I think this is indeed risky to have it. I would be against having it in this form - but I think it could be shown with masking. You'd just need to take the "masked" string as source for JSON formatter rather than the value of the variable and convert it back to json. I can't imagine "real" situation where masking would break json structure (It would hve to be a really weird combination of parameter containing a " } or similar weird combination (and we always can fall-back to plain string if json formatting fails).

@aa3pankaj aa3pankaj marked this pull request as draft February 18, 2022 19:26
@aa3pankaj aa3pankaj marked this pull request as ready for review February 19, 2022 16:19
@aa3pankaj aa3pankaj requested a review from ashb February 19, 2022 16:19
@aa3pankaj
Copy link
Contributor Author

@ashb please review, updated code for masking secret values

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Is it possible to truncate the value field in the listing table? Now that it’s easy to view the complete value, we can probably make the table a bit prettier for long values.

@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 Feb 22, 2022
@github-actions
Copy link

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 main or amend the last commit of the PR, and push it with --force-with-lease.

@aa3pankaj
Copy link
Contributor Author

aa3pankaj commented Feb 22, 2022

Using style { table-layout: fixed; word-wrap: break-word;} to fix below issue in case of long values:

Before:
Screenshot 2022-02-22 at 4 49 35 PM

After:
Screenshot 2022-02-22 at 4 51 51 PM

@aa3pankaj
Copy link
Contributor Author

aa3pankaj commented Feb 22, 2022

@uranusjr please review, updated code for truncating long values in 'List Variable' table:

Before:
Screenshot 2022-02-22 at 6 22 49 PM

After:
airflow_list_variable

@aa3pankaj aa3pankaj requested a review from uranusjr February 22, 2022 12:55
@potiuk
Copy link
Member

potiuk commented Feb 26, 2022

LGTM but I am not into the UI stuff: @uranusjr @ashb :)?

Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

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

UI looks good to me. I wonder if some of the styles can be applied to all of our tables but that doesn't need to be this PR.

@potiuk potiuk merged commit f0bbb9d into apache:main Feb 28, 2022
@jedcunningham jedcunningham added the type:improvement Changelog: Improvements label Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues okay to merge It's ok to merge this PR as it does not require more tests type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a 'View' button for Airflow Variables in the UI
7 participants