You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Prab-27 (via GitHub)" <gi...@apache.org> on 2025/07/13 17:09:51 UTC
[PR] AIP-81 : Abstracting Operation Calls Using a Generic Method [airflow]
Prab-27 opened a new pull request, #53323:
URL: https://github.com/apache/airflow/pull/53323
Related discussion: [here](https://github.com/apache/airflow/pull/50132#discussion_r2175408726
)
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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 [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-81 : Abstracting Operation Calls Using a Generic Method [airflow]
Posted by "Prab-27 (via GitHub)" <gi...@apache.org>.
Prab-27 commented on PR #53323:
URL: https://github.com/apache/airflow/pull/53323#issuecomment-3069276521
> Many thanks @Prab-27! I believe the intention of the comment in the other PR is just for list methods. I don’t think we need this much structural changes in the code just before release:)
Oh, do I need to remove it from all methods except list, and do it after release?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-81 : Abstracting Operation Calls Using a Generic Method [airflow]
Posted by "Prab-27 (via GitHub)" <gi...@apache.org>.
Prab-27 commented on PR #53323:
URL: https://github.com/apache/airflow/pull/53323#issuecomment-3073908388
@bugraoz93 Thanks a lot
No need to apologize—it’s also my fault. I should’ve clarified things earlier, and I’m not at a loss.
I learned a lot during the process of working on this PR.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-81 : Abstracting Operation Calls Using a Generic Method [airflow]
Posted by "Prab-27 (via GitHub)" <gi...@apache.org>.
Prab-27 commented on PR #53323:
URL: https://github.com/apache/airflow/pull/53323#issuecomment-3069280110
> Additionally, these pieces will be auto generated in the future so let's not make too much effort for now to just unify them
O Nice!!Just like commands ?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-81 : Abstracting Operation Calls Using a Generic Method [airflow]
Posted by "Prab-27 (via GitHub)" <gi...@apache.org>.
Prab-27 commented on PR #53323:
URL: https://github.com/apache/airflow/pull/53323#issuecomment-3073113458
@bugraoz93 Thanks!!
I'm happy to close this PR if we're close to release and the changes aren't essential at this point. What do you think ?
If any issues related to `airflowctl ` come up, feel free to tag me — I'd enjoy looking into them
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-81 : Abstracting Operation Calls Using a Generic Method [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #53323:
URL: https://github.com/apache/airflow/pull/53323#issuecomment-3073872584
> @bugraoz93 Thanks!!
> I'm happy to close this PR if we're close to release and the changes aren't essential at this point. What do you think ?
>
> If any issues related to `airflowctl ` , feel free to tag me — I'd enjoy looking into them
>
Many thanks @Prab-27 ! You are one of the sole contributors to airflowctl, I will ping you. After rc release, we would most probably need to work a bit on a couple of parts while testing more. Let's close this for now as even without release I cannot see the value it is bringing. Sorry if I misguided you which cost additional effort!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-81 : Abstracting Operation Calls Using a Generic Method [airflow]
Posted by "dominikhei (via GitHub)" <gi...@apache.org>.
dominikhei commented on code in PR #53323:
URL: https://github.com/apache/airflow/pull/53323#discussion_r2203557096
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -388,150 +361,104 @@ def bulk(self, connections: BulkBodyConnectionBody) -> BulkResponse | ServerResp
return BulkResponse.model_validate_json(self.response.content)
except ServerResponseError as e:
raise e
+ # return super().execute_operation(method="patch",url = "connections",data_model = BulkResponse,json=connections.model_dump())
def create_defaults(self) -> None | ServerResponseError:
"""Create default connections."""
- try:
- self.response = self.client.post("connections/defaults")
- return None
- except ServerResponseError as e:
- raise e
+ return super().execute_operation(method="post", url="connections/defaults")
def delete(self, conn_id: str) -> str | ServerResponseError:
"""Delete a connection."""
- try:
- self.client.delete(f"connections/{conn_id}")
- return conn_id
- except ServerResponseError as e:
- raise e
+ return super().execute_operation(method="delete", url=f"connections/{conn_id}")
def update(
self,
connection: ConnectionBody,
) -> ConnectionResponse | ServerResponseError:
"""Update a connection."""
- try:
- self.response = self.client.patch(
- f"connections/{connection.connection_id}", json=connection.model_dump()
- )
- return ConnectionResponse.model_validate_json(self.response.content)
- except ServerResponseError as e:
- raise e
+ return super().execute_operation(
+ method="patch",
+ url=f"connections/{connection.connection_id}",
+ data_model=ConnectionResponse,
+ json=connection.model_dump(),
+ )
def test(
self,
connection: ConnectionBody,
) -> ConnectionTestResponse | ServerResponseError:
"""Test a connection."""
- try:
- self.response = self.client.post("connections/test", json=connection.model_dump())
- return ConnectionTestResponse.model_validate_json(self.response.content)
- except ServerResponseError as e:
- raise e
+ return super().execute_operation(
+ method="post", url="connections/test", data_model=ConnectionTestResponse
+ )
class DagOperations(BaseOperations):
"""Dag operations."""
def get(self, dag_id: str) -> DAGResponse | ServerResponseError:
"""Get a DAG."""
- try:
- self.response = self.client.get(f"dags/{dag_id}")
- return DAGResponse.model_validate_json(self.response.content)
- except ServerResponseError as e:
- raise e
+ return super().execute_operation(method="get", url="dags/{dag_id}", data_model=DAGResponse)
Review Comment:
you forgot to make it into an f-string here :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-81 : Abstracting Operation Calls Using a Generic Method [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #53323:
URL: https://github.com/apache/airflow/pull/53323#issuecomment-3069406137
> > Many thanks @Prab-27! I believe the intention of the comment in the other PR is just for list methods. I don’t think we need this much structural changes in the code just before release:)
>
> Oh, do I need to remove it from all methods except list, and do it after release?
I think you can even include this in your other PR but this change would be more safe after merging other one where we listing all entities so that you don't need to do it again in one of them
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-81 : Abstracting Operation Calls Using a Generic Method [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #53323:
URL: https://github.com/apache/airflow/pull/53323#issuecomment-3069271191
Additionally, these pieces will be auto generated in the future so let's not make too much effort for now to just unify them
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-81 : Abstracting Operation Calls Using a Generic Method [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #53323:
URL: https://github.com/apache/airflow/pull/53323#discussion_r2204744214
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -147,6 +152,25 @@ def __init_subclass__(cls, **kwargs):
if callable(value):
setattr(cls, attr, _check_flag_and_exit_if_server_response_error(value))
+ def execute_operation(
+ self,
+ *,
+ method: str,
+ url: str,
+ data_model: type[BaseModel] | None = None,
+ return_entry: str | None = None,
+ **kwargs,
+ ):
+ try:
+ print("try")
Review Comment:
```suggestion
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-81 : Abstracting Operation Calls Using a Generic Method [airflow]
Posted by "Prab-27 (via GitHub)" <gi...@apache.org>.
Prab-27 closed pull request #53323: AIP-81 : Abstracting Operation Calls Using a Generic Method
URL: https://github.com/apache/airflow/pull/53323
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-81 : Abstracting Operation Calls Using a Generic Method [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #53323:
URL: https://github.com/apache/airflow/pull/53323#issuecomment-3069413348
Something like in list methods we will call from super and that's 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org