-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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 rest API to query for providers #13394
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
|
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*. |
6be98aa
to
50340af
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*. |
Docs also need to be fixed. |
50340af
to
4e53b6b
Compare
@potiuk You mean the step in the CI that was failing? Or i missed to update some documentation files? |
Yep. We just merged a fix for that AWS batch. If f you could rebase the last time (I hope) that would be great! |
4e53b6b
to
507bd69
Compare
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. |
I think we should refrain from merging it until we agree on how we approach 2.0.1 with branching: #13382 (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.
Looks good, but seems this is the first change to be merged that should be targeted to 2.1 so we need to decide how we approach it.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
@kaxil @ephraimbuddy This looks good to me, but can y'all confirm that this is still good to go? If so, we can merge it tomorrow. @nic314 Sorry this has been left in limbo for so long. Would you mind rebasing it onto master? |
507bd69
to
c4c5a6f
Compare
@jhtimmins I rebased, but looks like the cicd needs some approval. Also i was assuming this was for the version 2.1, let me know is i need to change here c4c5a6f#diff-191056f40fba6bf5886956aa281e0e0d2bb4ddaa380beb012d922a25f5c65750R136 |
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.
Just the tests, LGTM overrall
permissions=[(permissions.ACTION_CAN_READ, permissions.RESOURCE_PROVIDER)], # type: ignore | ||
) | ||
create_user(cls.app, username="test_no_permissions", role_name="TestNoPermissions") # type: ignore | ||
|
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.
A while ago, we made some changes to tests speedup. I'd appreciate a change of this test to use that. See https://github.com/apache/airflow/blob/b7b17641d0c17bfa5e1f8c77b9c232da73bbedcd/tests/api_connexion/endpoints/test_connection_endpoint.py
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 will do asap, i think within today
Hi @potiuk , is it time we merge this? |
The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason. |
@ephraimbuddy We can go ahead and merge, since this has targeted 2.1 and we're actively trying to get those PRs in. |
I just reverted the offending PR (and asked the author to fix also the reason why (#15703 (comment)) hey @jhtimmins - broken master happens from time to time for different reasons, so far we've been trying to fix such things (often by just pin-pointing and reverting such offending PRs'). I think we should keep doing it, Merging "quickly" something because we are in hurry is almost never a good idea. |
I think so far in Airflow we were doing really well about avoiding the "not my problem" attitude. |
@potiuk Thanks for the heads up about master, I missed that it was failing. As I understood it, Ephraim was asking whether we actually wanted this to be included for 2.1. As I'm in the middle of working on the 2.1 release, and am currently making a list of what will get into the release versus pushed to a later one, I was confirming that this feature should be included. Since the release date is approaching, I was encouraging him to be proactive about getting it fully reviewed and merged, not to cut any corners in quality or good practices. |
Sorry then :). I misunderstood that - It looked as if we are hurrying now for unknown reason :). |
Hey @nic314 - can you please rebase. I am also happy to merge it as soon as it is green! |
ed5cfe8
to
6bb77de
Compare
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.
Looks great! Thanks @nic314 . Let's hope all tests pass this time :).
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. |
looks like everything passed👍 |
Awesome work, congrats on your first merged pull request! |
Wohoo 🎉 ! |
closes: #12468
Added a new api rest endpoint in the stable api to query the installed providers. It provides the same information as the cli command "airflow providers list"
Note: There is some code duplication between provider_endpoint.py and provider_command.py regarding_remove_rst_syntax and the mapping, i kept it like this becouse i assume that in future its possible that the rest api and the cli returns different informations/fields but if that's not the case and the rest and cli will always return the same information then i can refactor my pr to avoid this duplication.