Skip to content

AIP-84 Migrate post a connection to FastAPI API - #43396

Merged
pierrejeambrun merged 8 commits into
apache:mainfrom
bugraoz93:feat/42593/post-connection-fastapi
Nov 5, 2024
Merged

AIP-84 Migrate post a connection to FastAPI API#43396
pierrejeambrun merged 8 commits into
apache:mainfrom
bugraoz93:feat/42593/post-connection-fastapi

Conversation

@bugraoz93

Copy link
Copy Markdown
Contributor

closes: #42593


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborgboring-cyborgBot added area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. labels Oct 26, 2024
@bugraoz93bugraoz93 added the legacy api Whether legacy API changes should be allowed in PR label Oct 26, 2024
Comment threadairflow/api_fastapi/core_api/serializers/connections.py Outdated

@jscheffljscheffl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One nit, else looks good.

@bugraoz93
bugraoz93force-pushed the feat/42593/post-connection-fastapi branch from c6c4784 to df9b0b4CompareOctober 27, 2024 00:55

@pierrejeambrunpierrejeambrun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we handle the password as well ? How do we plan to set the connection password through the rest api ?

(And check that in the response of the POST it is properly redacted).

ATM this is done trough FAB in the Adming/Connection form that allow the password field. But this will be removed in airflow 3.0 so we should handle it in the rest API. Also the doc states passwordhttps://airflow.apache.org/docs/apache-airflow/stable/stable-rest-api-ref.html#operation/post_connection but it was not handled I believe.

@pierrejeambrunpierrejeambrun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just to not merge by mistake until we clarify the password thing.

@bugraoz93

Copy link
Copy Markdown
ContributorAuthor

Should we handle the password as well ? How do we plan to set the connection password through the rest api ?

(And check that in the response of the POST it is properly redacted).

ATM this is done trough FAB in the Adming/Connection form that allow the password field. But this will be removed in airflow 3.0 so we should handle it in the rest API. Also the doc states passwordhttps://airflow.apache.org/docs/apache-airflow/stable/stable-rest-api-ref.html#operation/post_connection but it was not handled I believe.

Yes, we should. Indeed, the post and the patch include a password. Awesome one!
Let me make this work with the password as well as for the PATCH one. My mind ignored the password in the API. When I see passwords my mind just makes them disappear 😄

@bugraoz93
bugraoz93force-pushed the feat/42593/post-connection-fastapi branch from df9b0b4 to ffe3dbfCompareOctober 30, 2024 00:12
Comment threadtests/api_fastapi/core_api/routes/public/test_connections.py Outdated
@bugraoz93

bugraoz93 commented Nov 1, 2024

Copy link
Copy Markdown
ContributorAuthor

This still needs some work. I am looking
Edit: depends on the work on #43102

Comment threadairflow/api_fastapi/core_api/serializers/connections.py Outdated
@pierrejeambrun

Copy link
Copy Markdown
Member

We need to rebase to solve conflicts, but ready to merge IMO.

@bugraoz93
bugraoz93force-pushed the feat/42593/post-connection-fastapi branch from 9717f7a to 5630215CompareNovember 4, 2024 18:38
@bugraoz93

Copy link
Copy Markdown
ContributorAuthor

Good to hear that! Rebased and included the field into redact from the other PR comment :)

@bugraoz93

Copy link
Copy Markdown
ContributorAuthor

Something is off with the tests. These should have been caught by the CI/CD already.
FAILED providers/tests/fab/auth_manager/api_endpoints/test_backfill_endpoint.py
I think those parts are deleted maybe 🤔 it should be fixed in #43654.

@pierrejeambrun
pierrejeambrunforce-pushed the feat/42593/post-connection-fastapi branch from 5630215 to 5d50c7eCompareNovember 5, 2024 14:47
@pierrejeambrun
pierrejeambrun merged commit 1328d1a into apache:mainNov 5, 2024
ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024
* Migrate Create a Connection to FastAPI
* Remove additional duplicate comment
* Include password in connection and move dashboard.py to serializers/ui/
* Fix test for password
* Include password field to response and redact it, run pre-commit after rebase
* Convert redact to field_validator and fix tests
* Pass field name into redact
* run pre-commit after rebase
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:APIAirflow's REST/HTTP APIarea:UIRelated to UI/UX. For Frontend Developers.legacy apiWhether legacy API changes should be allowed in PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AIP-84 Migrate the public endpoint Post Connection to FastAPI

3 participants

@bugraoz93@pierrejeambrun@jscheffl