Migrate Edge calls for Worker to FastAPI part 4 - Cleanup#44434
Merged
jscheffl merged 2 commits intoapache:mainfrom Dec 1, 2024
Merged
Migrate Edge calls for Worker to FastAPI part 4 - Cleanup#44434jscheffl merged 2 commits intoapache:mainfrom
jscheffl merged 2 commits intoapache:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Copilot reviewed 11 out of 26 changed files in this pull request and generated 1 suggestion.
Files not reviewed (15)
- docs/spelling_wordlist.txt: Language not supported
- providers/src/airflow/providers/edge/CHANGELOG.rst: Language not supported
- providers/src/airflow/providers/edge/worker_api/routes/_v2_routes.py: Evaluated as low risk
- providers/src/airflow/providers/edge/worker_api/auth.py: Evaluated as low risk
- providers/src/airflow/providers/edge/worker_api/datamodels.py: Evaluated as low risk
- providers/src/airflow/providers/edge/worker_api/app.py: Evaluated as low risk
- providers/src/airflow/providers/edge/worker_api/routes/rpc_api.py: Evaluated as low risk
- providers/src/airflow/providers/edge/worker_api/routes/logs.py: Evaluated as low risk
- providers/src/airflow/providers/edge/worker_api/routes/jobs.py: Evaluated as low risk
- providers/src/airflow/providers/edge/init.py: Evaluated as low risk
- providers/src/airflow/providers/edge/worker_api/routes/_v2_compat.py: Evaluated as low risk
- providers/src/airflow/providers/edge/executors/edge_executor.py: Evaluated as low risk
- providers/src/airflow/providers/edge/models/edge_logs.py: Evaluated as low risk
- providers/src/airflow/providers/edge/models/edge_job.py: Evaluated as low risk
- providers/src/airflow/providers/edge/cli/edge_command.py: Evaluated as low risk
b575110 to
2c127df
Compare
kaxil
approved these changes
Nov 29, 2024
2c127df to
f6dc914
Compare
jscheffl
commented
Nov 30, 2024
providers/src/airflow/providers/edge/worker_api/routes/_v2_routes.py
Outdated
Show resolved
Hide resolved
This was referenced Nov 30, 2024
5a1db37 to
6ba555a
Compare
Contributor
There was a problem hiding this comment.
Copilot reviewed 5 out of 15 changed files in this pull request and generated no suggestions.
Files not reviewed (10)
- providers/src/airflow/providers/edge/CHANGELOG.rst: Language not supported
- providers/src/airflow/providers/edge/openapi/edge_worker_api_v1.yaml: Language not supported
- providers/tests/edge/models/test_edge_job.py: Evaluated as low risk
- providers/src/airflow/providers/edge/models/edge_logs.py: Evaluated as low risk
- providers/src/airflow/providers/edge/models/edge_job.py: Evaluated as low risk
- providers/src/airflow/providers/edge/cli/edge_command.py: Evaluated as low risk
- providers/src/airflow/providers/edge/models/edge_worker.py: Evaluated as low risk
- providers/tests/edge/cli/test_edge_command.py: Evaluated as low risk
- providers/src/airflow/providers/edge/worker_api/app.py: Evaluated as low risk
- providers/src/airflow/providers/edge/provider.yaml: Evaluated as low risk
Comments skipped due to low confidence (2)
providers/src/airflow/providers/edge/cli/api_client.py:113
- The docstring is incorrect. It should be updated to: """Update the state of the worker in the central site and thereby implicitly heartbeat."""
"""Register worker with the Edge API."""
providers/src/airflow/providers/edge/cli/api_client.py:128
- The method
model_dump_jsonis not a standard Pydantic method. Usejson()instead:WorkerQueuesBody(queues=queues, free_concurrency=free_concurrency).json(exclude_unset=True)
WorkerQueuesBody(queues=queues, free_concurrency=free_concurrency).model_dump_json(exclude_unset=True)
potiuk
reviewed
Nov 30, 2024
potiuk
approved these changes
Nov 30, 2024
Member
potiuk
left a comment
There was a problem hiding this comment.
There is some devilish trickery with this one... Seems like it's no change at all:
But ... It looks good:
This was referenced Dec 1, 2024
6ba555a to
e176682
Compare
Contributor
Author
Now after Part 3 is merged looks much better:
|
Member
:) |
got686-yandex
pushed a commit
to got686-yandex/airflow
that referenced
this pull request
Jan 30, 2025
* Remove internal API bindings after migration to FastAPI * Move import to function preventing module import errors
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After PR #44311, #44330 and #44433 this PR is a cleanup and removes the old Internal API extensions.
For review: Only the last commit is relevant, the other 7 are from referenced base PRs