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.

Sign up for GitHub

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

Jump to bottom

Added table to view providers in airflow ui under admin tab #15385

Merged
merged 18 commits into from
Aug 19, 2021
Merged

Added table to view providers in airflow ui under admin tab #15385

merged 18 commits into from
Aug 19, 2021

Conversation

ShakaibKhan
Copy link
Contributor

Added page in Airflow UI for provider package info
Screen Shot 2021-04-15 at 2 05 59 PM

@boring-cyborg boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Apr 15, 2021
@ShakaibKhan ShakaibKhan mentioned this pull request Apr 15, 2021
Closed
@xinbinhuang
Copy link
Contributor

@ShakaibKhan Thanks for the contribution! Love this.

Can you rebase to master, please :)

xinbinhuang
xinbinhuang reviewed Apr 16, 2021
View reviewed changes
airflow/www/views.py Outdated Show resolved Hide resolved
mik-laj
mik-laj reviewed Apr 16, 2021
View reviewed changes
airflow/www/views.py Outdated Show resolved Hide resolved
@ryanahamilton
Copy link
Contributor

Is there a way that we could parse the descriptions to convert the URLs (that most of them contain) to actual links?

@ShakaibKhan ShakaibKhan changed the title Added table to view providers in airflow ui under admin tab [WIP] Added table to view providers in airflow ui under admin tab Apr 28, 2021
@ShakaibKhan
Copy link
Contributor Author

Is there a way that we could parse the descriptions to convert the URLs (that most of them contain) to actual links?

Tried implementing this with adding some anchor tags around links but having issues with jinja adding quotes to multiline descriptions

jhtimmins
jhtimmins requested changes May 4, 2021
View reviewed changes
Copy link
Contributor

@jhtimmins jhtimmins left a comment

Choose a reason for hiding this comment

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

The PR looks great! Just a few minor comments to improve consistency/readability. We can merge this tomorrow if you're able to make the changes and will rebase to master.

airflow/security/permissions.py Outdated Show resolved Hide resolved
airflow/www/templates/airflow/providers.html Outdated Show resolved Hide resolved
airflow/www/views.py Outdated Show resolved Hide resolved
airflow/www/views.py Outdated Show resolved Hide resolved
airflow/www/views.py Outdated Show resolved Hide resolved
tests/www/test_views.py Outdated Show resolved Hide resolved
tests/www/test_views.py Outdated
class TestProvidersView(TestBase):
def test_should_list_providers_on_page_with_details(self):
resp = self.client.get('/providers')
self.check_content_in_response("Providers", resp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you validate that the necessary permissions are included?

@ShakaibKhan
Copy link
Contributor Author

Made readability changes and rebased but am uncertain how to check for permissions in the views_test.py

@jhtimmins
Copy link
Contributor

Made readability changes and rebased but am uncertain how to check for permissions in the views_test.py

Thanks @ShakaibKhan. You can call .create_user_and_login (as seen here) to create a user with specific permissions. So if you can call that in test_should_list_providers_on_page_with_details to give them the necessary permissions, and then add another test where you create a user but don't give them any permissions, so they've authenticated but not authorized.

@jhtimmins jhtimmins changed the title [WIP] Added table to view providers in airflow ui under admin tab Added table to view providers in airflow ui under admin tab May 6, 2021
@github-actions
Copy link

github-actions bot commented May 6, 2021

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.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label May 6, 2021
@jhtimmins jhtimmins self-requested a review May 7, 2021 02:36
@jhtimmins
Copy link
Contributor

@ShakaibKhan Looks like a test is failing since you added another view that adds an additional query. If you can just increment the number of expected queries in the test that should fix it.

@github-actions
Copy link

github-actions bot commented May 8, 2021

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@jedcunningham
Copy link
Member

@ShakaibKhan, looks like tests/www/views/test_views_base.py::test_index is failing due to query count. Your +1 to it probably got lost during your last rebase.

@jedcunningham jedcunningham dismissed ashb’s stale review August 19, 2021 16:12

The monolithic test_view.py is no longer being brought back.

@kaxil kaxil merged commit 12709bf into apache:main Aug 19, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Aug 19, 2021

Awesome work, congrats on your first merged pull request!

@kaxil
Copy link
Member

kaxil commented Aug 19, 2021

Thank you for your contribution @ShakaibKhan

@potiuk
Copy link
Member

potiuk commented Aug 19, 2021

❤️ it

@potiuk potiuk mentioned this pull request Aug 19, 2021
Merged
potiuk added a commit to potiuk/airflow that referenced this pull request Aug 19, 2021
@potiuk
Add links to provider's documentation
2bdd55b 
The provider package names in the UI are now linked to the
documentation of the provider (in the exact version provider is
installed in!).

Follow up after apache#15385
potiuk added a commit that referenced this pull request Aug 20, 2021
@potiuk
Add links to provider's documentation (#17736)
4fb184b 
The provider package names in the UI are now linked to the
documentation of the provider (in the exact version provider is
installed in!).

Follow up after #15385
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaxil kaxil kaxil approved these changes

@mik-laj mik-laj mik-laj left review comments

@xinbinhuang xinbinhuang xinbinhuang left review comments

@potiuk potiuk potiuk approved these changes

@jedcunningham jedcunningham jedcunningham approved these changes

@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton ryanahamilton is a code owner

@jhtimmins jhtimmins Awaiting requested review from jhtimmins

@dimberman dimberman Awaiting requested review from dimberman

@ephraimbuddy ephraimbuddy Awaiting requested review from ephraimbuddy

@turbaszek turbaszek Awaiting requested review from turbaszek

@vikramkoka vikramkoka Awaiting requested review from vikramkoka

@XD-DENG XD-DENG Awaiting requested review from XD-DENG

@ashb ashb Awaiting requested review from ashb ashb is a code owner

Assignees
No one assigned
Labels
area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Milestone
Airflow 2.2.0
Development

Successfully merging this pull request may close these issues.

Display providers in Web UI
9 participants
@ShakaibKhan @xinbinhuang @ryanahamilton @jhtimmins @potiuk @jedcunningham @kaxil @ashb @mik-laj