Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
bugraoz93 commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4643107931 #68175 closing in favor of this, @ElangoSubramani will work on next step migration commands :raised_hands: -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
bugraoz93 closed pull request #66261: feat: Enhance local client API with AirflowCtlClient integration (#64… URL: https://github.com/apache/airflow/pull/66261 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
bugraoz93 commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4643106351 Thanks for your understanding, @ElangoSubramani! It indeed requires a bit of in-depth understanding of this. I will tag you on those migration tasks after merging the client code. I created a PR for it. If internally agreed on the auth approach, it should be good. Then we need to migrate remote commands, so there will be migration work will be created under the same project. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
ElangoSubramani commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4642809187 I think I need more understanding about this module. No issues, if anything else I can pick up and work on let me know. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
bugraoz93 commented on code in PR #66261: URL: https://github.com/apache/airflow/pull/66261#discussion_r3369406728 ## airflow-core/src/airflow/api/client/local_client.py: ## @@ -34,7 +42,31 @@ def __init__(self, auth=None, session: httpx.Client | None = None): self._session: httpx.Client = session or httpx.Client() if auth: self._session.auth = auth +self.api_token = self._create_api_token() + +def _create_api_token(self) -> str: +try: +auth_manager = get_auth_manager() +except RuntimeError: +auth_manager = init_auth_manager() +return auth_manager.generate_jwt( +user=SimpleAuthManagerUser(username=getuser(), role="admin"), Review Comment: This hasn't been resolved. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
bugraoz93 commented on code in PR #66261: URL: https://github.com/apache/airflow/pull/66261#discussion_r3369406122 ## airflow-core/src/airflow/api/client/local_client.py: ## @@ -19,22 +19,77 @@ from __future__ import annotations +import json +from datetime import datetime +from typing import Any + import httpx +from airflowctl.api.client import ClientKind as AirflowCtlClientKind, ServerResponseError, provide_api_client +from airflowctl.api.datamodels.generated import PoolBody, TriggerDAGRunPostBody +from airflowctl.exceptions import AirflowCtlConnectionException +from pydantic import BaseModel, ConfigDict, Field, ValidationError as PydanticValidationError from airflow.api.common import delete_dag, trigger_dag +from airflow.api_fastapi.app import get_auth_manager, init_auth_manager +from airflow.api_fastapi.auth.managers.simple.user import SimpleAuthManagerUser +from airflow.configuration import conf from airflow.exceptions import AirflowBadRequest, PoolNotFound from airflow.models.pool import Pool +from airflow.utils.platform import getuser from airflow.utils.types import DagRunTriggeredByType +class LocalDagRunResponse(BaseModel): +"""Dag Run response returned by the local fallback client.""" + +model_config = ConfigDict(from_attributes=True, populate_by_name=True) + +conf: dict[str, Any] | None +dag_id: str +dag_run_id: str = Field(validation_alias="run_id") +data_interval_start: datetime | None +data_interval_end: datetime | None +end_date: datetime | None +last_scheduling_decision: datetime | None +logical_date: datetime | None +run_type: str +start_date: datetime | None +state: str +triggering_user_name: str | None + + class Client: """Local API client implementation.""" def __init__(self, auth=None, session: httpx.Client | None = None): self._session: httpx.Client = session or httpx.Client() if auth: self._session.auth = auth +self.api_token = self._create_api_token() + +def _create_api_token(self) -> str: +try: +auth_manager = get_auth_manager() +except RuntimeError: +auth_manager = init_auth_manager() +return auth_manager.generate_jwt( +user=SimpleAuthManagerUser(username=getuser(), role="admin"), Review Comment: It is resolved, but this should be `BaseUser`, as stated in my May 3 comment. This still points to simpleauthmanageruser. If airflow constructed with another auth manager in the instance running the CLI can end up with other users too. https://github.com/apache/airflow/pull/66261/changes#r3178497850 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
ElangoSubramani commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4642775744 Sure @bugraoz93. Please work on this. I'll start from good first issue. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
bugraoz93 commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4642761283 This piece of work was entry point of migration and it is blocking entire aip 94 work for a month now. We haven't migrated single command while in 9 days we have code freeze for 3.3. If we cannot merge this in a day, unfortunately I will work on this as no other PRs for this one. We cannot mitrate any command otherwise, we should have at least base so that in first minor we should be able to backport migrations that won't be there for 3.3 as planned. After this we will be able to work all the commands in paralllel which will be smaller tasks few line changes. I would guide you to take simpler tasks like those where issues generally includes labels something `good first issue`. Those are easier to finish and generally can make your contributions faster to in and iterate over multiple parts to learn multiple places. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
bugraoz93 commented on code in PR #66261: URL: https://github.com/apache/airflow/pull/66261#discussion_r3369380035 ## airflow-core/src/airflow/api/client/local_client.py: ## @@ -19,22 +19,77 @@ from __future__ import annotations +import json +from datetime import datetime +from typing import Any + import httpx +from airflowctl.api.client import ClientKind as AirflowCtlClientKind, ServerResponseError, provide_api_client +from airflowctl.api.datamodels.generated import PoolBody, TriggerDAGRunPostBody +from airflowctl.exceptions import AirflowCtlConnectionException +from pydantic import BaseModel, ConfigDict, Field, ValidationError as PydanticValidationError from airflow.api.common import delete_dag, trigger_dag +from airflow.api_fastapi.app import get_auth_manager, init_auth_manager +from airflow.api_fastapi.auth.managers.simple.user import SimpleAuthManagerUser +from airflow.configuration import conf from airflow.exceptions import AirflowBadRequest, PoolNotFound from airflow.models.pool import Pool +from airflow.utils.platform import getuser from airflow.utils.types import DagRunTriggeredByType +class LocalDagRunResponse(BaseModel): +"""Dag Run response returned by the local fallback client.""" + +model_config = ConfigDict(from_attributes=True, populate_by_name=True) + +conf: dict[str, Any] | None +dag_id: str +dag_run_id: str = Field(validation_alias="run_id") +data_interval_start: datetime | None +data_interval_end: datetime | None +end_date: datetime | None +last_scheduling_decision: datetime | None +logical_date: datetime | None +run_type: str +start_date: datetime | None +state: str +triggering_user_name: str | None + + Review Comment: We are still creating new client here and it is not even airflowctl driven child class. We should create airflowctl client. What i mean singleton client is airflowctl client va generated token and all of them using single aggregation object even though we don't change class structure but since we are changing we shpuld do that those are consequences of using singleton airflowctl rather maintaining huge class here as we are currently have those in ctl with exception handled. We should revise accordingly and use api datamodels -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
bugraoz93 commented on code in PR #66261: URL: https://github.com/apache/airflow/pull/66261#discussion_r3369372973 ## airflow-core/src/airflow/api/client/local_client.py: ## @@ -19,22 +19,77 @@ from __future__ import annotations +import json +from datetime import datetime +from typing import Any + import httpx +from airflowctl.api.client import ClientKind as AirflowCtlClientKind, ServerResponseError, provide_api_client +from airflowctl.api.datamodels.generated import PoolBody, TriggerDAGRunPostBody +from airflowctl.exceptions import AirflowCtlConnectionException +from pydantic import BaseModel, ConfigDict, Field, ValidationError as PydanticValidationError from airflow.api.common import delete_dag, trigger_dag +from airflow.api_fastapi.app import get_auth_manager, init_auth_manager +from airflow.api_fastapi.auth.managers.simple.user import SimpleAuthManagerUser +from airflow.configuration import conf from airflow.exceptions import AirflowBadRequest, PoolNotFound from airflow.models.pool import Pool +from airflow.utils.platform import getuser from airflow.utils.types import DagRunTriggeredByType +class LocalDagRunResponse(BaseModel): Review Comment: We should use it from API directly. We shouldn't create anything new -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
ElangoSubramani commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4623275072 I have updated it can you please review it -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
bugraoz93 commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4617298130 As part of ticket it would be even enough to create the client singleton object and we can split other works through tickets and multiple PRs -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
bugraoz93 commented on code in PR #66261: URL: https://github.com/apache/airflow/pull/66261#discussion_r3351045964 ## airflow-core/src/airflow/api/client/local_client.py: ## @@ -70,20 +132,56 @@ def trigger_dag( } return dag_run -def delete_dag(self, dag_id): +@provide_api_client(kind=AirflowCtlClientKind.CLI) Review Comment: We can get rid of all the code and convert usage methods to airflowctl ideally with client rahter updating these class methods. Please double validate this as we should create singleton client and use it accross CLI, this is generator reading from -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
bugraoz93 commented on code in PR #66261:
URL: https://github.com/apache/airflow/pull/66261#discussion_r3351028213
##
airflow-core/src/airflow/api/client/local_client.py:
##
@@ -43,7 +75,37 @@ def trigger_dag(
logical_date=None,
triggering_user_name=None,
replace_microseconds=True,
+api_client=None,
) -> dict | None:
+if api_client is not None:
+parsed_conf = conf
+if isinstance(conf, str):
+try:
+parsed_conf = json.loads(conf)
+except json.JSONDecodeError as err:
+raise AirflowBadRequest(f"Invalid configuration JSON:
{err}") from err
+dag_run = api_client.dags.trigger(
+dag_id,
+TriggerDAGRunPostBody(
+dag_run_id=run_id,
+conf=parsed_conf,
+logical_date=logical_date,
+),
+)
+return {
Review Comment:
We should use again pydantic model validate here
##
airflow-core/src/airflow/api/client/local_client.py:
##
@@ -34,7 +42,31 @@ def __init__(self, auth=None, session: httpx.Client | None =
None):
self._session: httpx.Client = session or httpx.Client()
if auth:
self._session.auth = auth
+self.api_token = self._create_api_token()
Review Comment:
We shpuld get rid of this class. We can have a single variable using token
we should create singleton AirflowCtlClient and all of them use it. The
decorator is for auth flow
##
airflow-core/src/airflow/api/client/local_client.py:
##
@@ -70,20 +132,56 @@ def trigger_dag(
}
return dag_run
-def delete_dag(self, dag_id):
+@provide_api_client(kind=AirflowCtlClientKind.CLI)
Review Comment:
If this work we can get rid of all the code and convert usage methods to
airflowctl ideally with client. Please double validate this as we should create
singleton client and use it accross CLI, this is generator reading from
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
bugraoz93 commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4615628404 Good to hear if you install uv and prek remaining can come from running the test with `uv pytest -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
ElangoSubramani commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4615617951 > This part is important. Find this insertion and validate existence or do not insert at all > > ```shell > sqlite3.IntegrityError) UNIQUE constraint failed: dag.dag_id > [SQL: INSERT INTO dag (dag_id, is_paused, is_stale, > ``` Thanks for the guidance. I'm not used to this CI logs and was not able to replicate in local. But this helps -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
bugraoz93 commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4615597540 This part is important. Find this insertion and validate existence or do not insert at all ```shell sqlite3.IntegrityError) UNIQUE constraint failed: dag.dag_id [SQL: INSERT INTO dag (dag_id, is_paused, is_stale, ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
bugraoz93 commented on PR #66261:
URL: https://github.com/apache/airflow/pull/66261#issuecomment-4615583951
Checked one of them but error seems self explanatory in CI
```shell
(Background on this error at: https://sqlalche.me/e/20/gkpj) (Background on
this error at: https://sqlalche.me/e/20/7s2a)
ERROR
airflow-core/tests/unit/cli/commands/test_dag_command.py::TestCliDags::test_delete_dag_existing_file
- sqlalchemy.exc.PendingRollbackError: This Session's transaction has been
rolled back due to a previous exception during flush. To begin a new
transaction with this Session, first issue Session.rollback(). Original
exception was: (sqlite3.IntegrityError) UNIQUE constraint failed: dag.dag_id
[SQL: INSERT INTO dag (dag_id, is_paused, is_stale,
exceeds_max_non_backfill, last_parsed_time, last_parse_duration, last_expired,
fileloc, relative_fileloc, bundle_name, bundle_version, owners,
dag_display_name, description, timetable_type, timetable_summary,
timetable_description, max_active_tasks, max_active_runs,
max_consecutive_failed_dag_runs, has_task_concurrency_limits,
has_import_errors, fail_fast, next_dagrun, next_dagrun_data_interval_start,
next_dagrun_data_interval_end, next_dagrun_partition_key,
next_dagrun_partition_date, next_dagrun_create_after) VALUES (?, ?, ?, ?, ?, ?,
?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) RETURNING
timetable_partitioned, timetable_periodic]
[parameters: ('my_dag_id', 0, 1, 0, None, None, None,
'/tmp/pytest-of-root/pytest-0/test_delete_dag_existing_file0/testfile', None,
'dags-folder', None, None, None, None, '', None, None, 16, 16, 0, 1, 0, 0,
None, None, None, None, None, None)]
(Background on this error at: https://sqlalche.me/e/20/gkpj) (Background on
this error at: https://sqlalche.me/e/20/7s2a)
```
You are trying to insert something already exists in the database in the
tests so we should either expect for specific test case for this violation. It
seems before delete we are trying to insert which is already there
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
ElangoSubramani commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4615579604 Yes, Thanks -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
bugraoz93 commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4615560180 > Hi @bugraoz93 , > Yes I'm trying to fix the issues > Can you please respond for this conversation? > https://github.com/apache/airflow/pull/66261#pullrequestreview-4216621278 Thanks for swift reply! The link is going to my last review. Did you mean this https://github.com/apache/airflow/pull/66261#issuecomment-4460963236? Wiill check the problem soon after releasing 0.1.5 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
ElangoSubramani commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4615500062 Hi @bugraoz93 , Yes I'm trying to fix the issues Can you please respond for this conversation? https://github.com/apache/airflow/pull/66261#pullrequestreview-4216621278 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
bugraoz93 commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4615323027 Hey @ElangoSubramani, when could you finish this? Otherwise, please let me know, I tend to finish and merge pretty soon. This is blocking the entire project to migrate towards airflowctl :( -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
ElangoSubramani commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4530274167 Looking into it -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
ElangoSubramani commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4530272752 Sure -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
potiuk commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4529640465 @ElangoSubramani A few things need addressing before review — see our [Pull Request quality criteria](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-quality-criteria). - :x: **CI fails**: `Low dep tests:core / All-core:LowestDeps:14:3.10:API...CLI` (and possibly other checks — see the [Checks tab](https://github.com/apache/airflow/pull/66261/checks) for the full list). No rush. --- _Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this [two-stage triage process](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md#why-the-first-pass-is-automated) so that our maintainers' limited time is spent where it matters most: the conversation with you._ --- Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
ElangoSubramani commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4460963236 Can anyone please guide me to solve the test issue? ( I have posted in community slack also but not getting any help) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
ElangoSubramani commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4418588646 Thanks @potiuk for the information. Will update it and make it ready for review. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
potiuk commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4417481332 @ElangoSubramani Converting to **draft** — this PR doesn't yet meet our [Pull Request quality criteria](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-quality-criteria). - :x: **Provider tests** — Failing: 3 `provider distributions tests / Compat` runs against Airflow 3.0.6, 3.1.8, 3.2.1 (P3.10). See [docs](https://github.com/apache/airflow/blob/main/contributing-docs/12_provider_distributions.rst). - :x: **Unresolved review comments** — 1 unresolved review thread from @bugraoz93. See [docs](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-quality-criteria). See the linked criteria for how to fix each item, then mark the PR "Ready for review". This is **not** a rejection — just an invitation to bring the PR up to standard. No rush. --- _Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this [two-stage triage process](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md#why-the-first-pass-is-automated) so that our maintainers' limited time is spent where it matters most: the conversation with you._ --- Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
ElangoSubramani commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4368958300 https://github.com/user-attachments/assets/266d5a12-244b-479d-9e91-ef8093e7e30a"; /> I'm getting this warning while running unit test -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
henry3260 commented on code in PR #66261: URL: https://github.com/apache/airflow/pull/66261#discussion_r3178518926 ## airflow-core/src/airflow/api/client/local_client.py: ## @@ -43,7 +75,34 @@ def trigger_dag( logical_date=None, triggering_user_name=None, replace_microseconds=True, +api_client=None, ) -> dict | None: +if api_client is not None: +parsed_conf = conf +if isinstance(conf, str): +parsed_conf = json.loads(conf) Review Comment: Could we use a try-except block here to catch `json.JSONDecodeError`? It would make the cli more robust against invalid input strings. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
bugraoz93 commented on code in PR #66261: URL: https://github.com/apache/airflow/pull/66261#discussion_r3178497850 ## airflow-core/src/airflow/api/client/local_client.py: ## @@ -34,7 +42,31 @@ def __init__(self, auth=None, session: httpx.Client | None = None): self._session: httpx.Client = session or httpx.Client() if auth: self._session.auth = auth +self.api_token = self._create_api_token() + +def _create_api_token(self) -> str: +try: +auth_manager = get_auth_manager() +except RuntimeError: +auth_manager = init_auth_manager() +return auth_manager.generate_jwt( +user=SimpleAuthManagerUser(username=getuser(), role="admin"), Review Comment: I think this should be `baseUser` instead, otherwise for example Fab or Keycloak can fail if configured -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
ElangoSubramani commented on code in PR #66261:
URL: https://github.com/apache/airflow/pull/66261#discussion_r3176537236
##
airflow-core/src/airflow/api/client/local_client.py:
##
@@ -19,21 +19,84 @@
from __future__ import annotations
+import asyncio
+import json
+from contextlib import AsyncExitStack
+from functools import cache, cached_property
+
import httpx
+from a2wsgi import ASGIMiddleware
+from airflowctl.api.client import Client as AirflowCtlClient
+from airflowctl.api.client import ClientKind as AirflowCtlClientKind
+from airflowctl.api.client import ServerResponseError
+from airflowctl.api.datamodels.generated import PoolBody, TriggerDAGRunPostBody
+from airflow.api_fastapi.auth.managers.simple.user import SimpleAuthManagerUser
from airflow.api.common import delete_dag, trigger_dag
+from airflow.api_fastapi.app import cached_app
+from airflow.configuration import conf
from airflow.exceptions import AirflowBadRequest, PoolNotFound
from airflow.models.pool import Pool
+from airflow.utils.platform import getuser
from airflow.utils.types import DagRunTriggeredByType
+class _InProcessCoreAPI:
+@cached_property
+def app(self):
+return cached_app(apps="core")
+
+@cached_property
+def transport(self) -> httpx.WSGITransport:
+middleware = ASGIMiddleware(self.app)
+
+async def start_lifespan(cm: AsyncExitStack):
+await
cm.enter_async_context(self.app.router.lifespan_context(self.app))
+
+self._cm = AsyncExitStack()
+asyncio.run_coroutine_threadsafe(start_lifespan(self._cm),
middleware.loop)
+return httpx.WSGITransport(app=middleware)
+
+
+@cache
+def _in_process_core_api() -> _InProcessCoreAPI:
+return _InProcessCoreAPI()
+
+
class Client:
"""Local API client implementation."""
def __init__(self, auth=None, session: httpx.Client | None = None):
self._session: httpx.Client = session or httpx.Client()
if auth:
self._session.auth = auth
+self._airflowctl_client = self._create_airflowctl_client()
+
+def _create_airflowctl_client(self) -> AirflowCtlClient | None:
+app = _in_process_core_api().app
+auth_manager = app.state.auth_manager
+token = auth_manager.generate_jwt(
+user=SimpleAuthManagerUser(username=getuser(), role="admin"),
+expiration_time_in_seconds=conf.getint("api_auth",
"jwt_cli_expiration_time"),
+)
+return AirflowCtlClient(
+base_url=conf.get("api", "base_url",
fallback="http://localhost:8080";) or "http://localhost:8080";,
+token=token,
+kind=AirflowCtlClientKind.CLI,
+transport=_in_process_core_api().transport,
+)
+
Review Comment:
Sure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
ElangoSubramani commented on code in PR #66261: URL: https://github.com/apache/airflow/pull/66261#discussion_r3176535836 ## airflow-core/src/airflow/api/client/local_client.py: ## @@ -19,21 +19,84 @@ from __future__ import annotations +import asyncio +import json +from contextlib import AsyncExitStack +from functools import cache, cached_property + import httpx +from a2wsgi import ASGIMiddleware +from airflowctl.api.client import Client as AirflowCtlClient +from airflowctl.api.client import ClientKind as AirflowCtlClientKind +from airflowctl.api.client import ServerResponseError +from airflowctl.api.datamodels.generated import PoolBody, TriggerDAGRunPostBody +from airflow.api_fastapi.auth.managers.simple.user import SimpleAuthManagerUser from airflow.api.common import delete_dag, trigger_dag +from airflow.api_fastapi.app import cached_app +from airflow.configuration import conf from airflow.exceptions import AirflowBadRequest, PoolNotFound from airflow.models.pool import Pool +from airflow.utils.platform import getuser from airflow.utils.types import DagRunTriggeredByType +class _InProcessCoreAPI: +@cached_property +def app(self): +return cached_app(apps="core") + +@cached_property +def transport(self) -> httpx.WSGITransport: +middleware = ASGIMiddleware(self.app) + +async def start_lifespan(cm: AsyncExitStack): +await cm.enter_async_context(self.app.router.lifespan_context(self.app)) + +self._cm = AsyncExitStack() +asyncio.run_coroutine_threadsafe(start_lifespan(self._cm), middleware.loop) +return httpx.WSGITransport(app=middleware) Review Comment: Hi @jason810496 , Thanks for the review Can I remove it then? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
jason810496 commented on code in PR #66261:
URL: https://github.com/apache/airflow/pull/66261#discussion_r3176499721
##
airflow-core/src/airflow/api/client/local_client.py:
##
@@ -19,21 +19,84 @@
from __future__ import annotations
+import asyncio
+import json
+from contextlib import AsyncExitStack
+from functools import cache, cached_property
+
import httpx
+from a2wsgi import ASGIMiddleware
+from airflowctl.api.client import Client as AirflowCtlClient
+from airflowctl.api.client import ClientKind as AirflowCtlClientKind
+from airflowctl.api.client import ServerResponseError
+from airflowctl.api.datamodels.generated import PoolBody, TriggerDAGRunPostBody
+from airflow.api_fastapi.auth.managers.simple.user import SimpleAuthManagerUser
from airflow.api.common import delete_dag, trigger_dag
+from airflow.api_fastapi.app import cached_app
+from airflow.configuration import conf
from airflow.exceptions import AirflowBadRequest, PoolNotFound
from airflow.models.pool import Pool
+from airflow.utils.platform import getuser
from airflow.utils.types import DagRunTriggeredByType
+class _InProcessCoreAPI:
+@cached_property
+def app(self):
+return cached_app(apps="core")
+
+@cached_property
+def transport(self) -> httpx.WSGITransport:
+middleware = ASGIMiddleware(self.app)
+
+async def start_lifespan(cm: AsyncExitStack):
+await
cm.enter_async_context(self.app.router.lifespan_context(self.app))
+
+self._cm = AsyncExitStack()
+asyncio.run_coroutine_threadsafe(start_lifespan(self._cm),
middleware.loop)
+return httpx.WSGITransport(app=middleware)
Review Comment:
Base on the Client usage in CTL, it seems we don't need `transport` to
construct the `Client` class.
Perhaps we don't need to define `_InProcessCoreAPI` at all.
##
airflow-core/src/airflow/api/client/local_client.py:
##
@@ -19,21 +19,84 @@
from __future__ import annotations
+import asyncio
+import json
+from contextlib import AsyncExitStack
+from functools import cache, cached_property
+
import httpx
+from a2wsgi import ASGIMiddleware
+from airflowctl.api.client import Client as AirflowCtlClient
+from airflowctl.api.client import ClientKind as AirflowCtlClientKind
+from airflowctl.api.client import ServerResponseError
+from airflowctl.api.datamodels.generated import PoolBody, TriggerDAGRunPostBody
+from airflow.api_fastapi.auth.managers.simple.user import SimpleAuthManagerUser
from airflow.api.common import delete_dag, trigger_dag
+from airflow.api_fastapi.app import cached_app
+from airflow.configuration import conf
from airflow.exceptions import AirflowBadRequest, PoolNotFound
from airflow.models.pool import Pool
+from airflow.utils.platform import getuser
from airflow.utils.types import DagRunTriggeredByType
+class _InProcessCoreAPI:
+@cached_property
+def app(self):
+return cached_app(apps="core")
+
+@cached_property
+def transport(self) -> httpx.WSGITransport:
+middleware = ASGIMiddleware(self.app)
+
+async def start_lifespan(cm: AsyncExitStack):
+await
cm.enter_async_context(self.app.router.lifespan_context(self.app))
+
+self._cm = AsyncExitStack()
+asyncio.run_coroutine_threadsafe(start_lifespan(self._cm),
middleware.loop)
+return httpx.WSGITransport(app=middleware)
+
+
+@cache
+def _in_process_core_api() -> _InProcessCoreAPI:
+return _InProcessCoreAPI()
+
+
class Client:
"""Local API client implementation."""
def __init__(self, auth=None, session: httpx.Client | None = None):
self._session: httpx.Client = session or httpx.Client()
if auth:
self._session.auth = auth
+self._airflowctl_client = self._create_airflowctl_client()
+
+def _create_airflowctl_client(self) -> AirflowCtlClient | None:
+app = _in_process_core_api().app
+auth_manager = app.state.auth_manager
+token = auth_manager.generate_jwt(
+user=SimpleAuthManagerUser(username=getuser(), role="admin"),
+expiration_time_in_seconds=conf.getint("api_auth",
"jwt_cli_expiration_time"),
+)
Review Comment:
How about using `get_auth_manager().generate_jwt(...)` instead of relying on
the FastAPI app.
##
airflow-core/src/airflow/api/client/local_client.py:
##
@@ -19,21 +19,84 @@
from __future__ import annotations
+import asyncio
+import json
+from contextlib import AsyncExitStack
+from functools import cache, cached_property
+
import httpx
+from a2wsgi import ASGIMiddleware
+from airflowctl.api.client import Client as AirflowCtlClient
+from airflowctl.api.client import ClientKind as AirflowCtlClientKind
+from airflowctl.api.client import ServerResponseError
+from airflowctl.api.datamodels.generated import PoolBody, TriggerDAGRunPostBody
+from airflow.api_fastapi.auth.managers.simple.user import SimpleAuthManagerUser
from airflow.api.
Re: [PR] feat: Enhance local client API with AirflowCtlClient integration (#64… [airflow]
boring-cyborg[bot] commented on PR #66261: URL: https://github.com/apache/airflow/pull/66261#issuecomment-4363545771 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 [Contributors' Guide](https://github.com/apache/airflow/blob/main/contributing-docs/README.rst) Here are some useful points: - Pay attention to the quality of your code (ruff, mypy and type annotations). Our [prek-hooks]( https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst#prerequisites-for-prek-hooks) will help you with that. - In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/airflow-core/docs/howto/custom-operator.rst) Consider adding an example Dag that shows how users should use it. - Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/dev/breeze/doc/README.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations. - Be patient and persistent. It might take some time to get a review or get the final approval from Committers. - Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack. - Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#coding-style-and-best-practices). - Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits. Apache Airflow is a community-driven project and together we are making it better 🚀. In case of doubts contact the developers at: Mailing List: [email protected] Slack: https://s.apache.org/airflow-slack -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
