You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ephraimbuddy (via GitHub)" <gi...@apache.org> on 2024/10/29 19:18:34 UTC
[PR] AIP-65: Make DAG source endpoint version aware [airflow]
ephraimbuddy opened a new pull request, #43492:
URL: https://github.com/apache/airflow/pull/43492
This enables us to get the dag code using dag ID, version name and
version number
Only the last commit matters.
Depends on #42913
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1843401295
##########
airflow/ui/src/pages/DagsList/Dag/Code/Code.tsx:
##########
@@ -55,13 +55,9 @@ export const Code = () => {
data: code,
error: codeError,
isLoading: isCodeLoading,
- } = useDagSourceServiceGetDagSource(
- {
- fileToken: dag?.file_token ?? "",
- },
- undefined,
- { enabled: Boolean(dag?.file_token) },
- );
+ } = useDagSourceServiceGetDagSource({
+ dagId: dagId ?? "",
Review Comment:
I left this to @bbovenzi and @pierrejeambrun to implement. I'm not sure how the version should be listed, selected and sent over through the API
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1833505734
##########
airflow/models/dag_version.py:
##########
@@ -136,7 +136,8 @@ def get_latest_version(cls, dag_id: str, *, session: Session = NEW_SESSION) -> D
def get_version(
cls,
dag_id: str,
- version_number: int = 1,
+ version_number: int | None = None,
+ version_name: str | None = None,
Review Comment:
I think it should either require it to be singular `get_version(version_number)`, or plural `get_versions(version_name)`. As proposed, getting the latest version for `get_version(version_name)` feels like a weird interface to me.
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1833828469
##########
airflow/models/dag_version.py:
##########
@@ -136,7 +136,8 @@ def get_latest_version(cls, dag_id: str, *, session: Session = NEW_SESSION) -> D
def get_version(
cls,
dag_id: str,
- version_number: int = 1,
+ version_number: int | None = None,
+ version_name: str | None = None,
Review Comment:
From the UI perspective, how should users search for DAG version? Should it be dag_id and number only?
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1847045778
##########
airflow/api_fastapi/core_api/routes/public/dag_sources.py:
##########
@@ -54,22 +53,24 @@
},
response_model=DAGSourceResponse,
)
-def get_dag_source(
- file_token: str,
+async def get_dag_source(
Review Comment:
or perhaps let's just start using the async sessions now @pierrejeambrun ?
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1843403099
##########
airflow/ui/src/pages/DagsList/Dag/Code/Code.tsx:
##########
@@ -55,13 +55,9 @@ export const Code = () => {
data: code,
error: codeError,
isLoading: isCodeLoading,
- } = useDagSourceServiceGetDagSource(
- {
- fileToken: dag?.file_token ?? "",
- },
- undefined,
- { enabled: Boolean(dag?.file_token) },
- );
+ } = useDagSourceServiceGetDagSource({
+ dagId: dagId ?? "",
Review Comment:
sounds good
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1842185797
##########
airflow/models/dag_version.py:
##########
@@ -136,7 +136,8 @@ def get_latest_version(cls, dag_id: str, *, session: Session = NEW_SESSION) -> D
def get_version(
cls,
dag_id: str,
- version_number: int = 1,
+ version_number: int | None = None,
+ version_name: str | None = None,
Review Comment:
I have removed the version_name altogether.
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1843361431
##########
airflow/ui/src/pages/DagsList/Dag/Code/Code.tsx:
##########
@@ -55,13 +55,9 @@ export const Code = () => {
data: code,
error: codeError,
isLoading: isCodeLoading,
- } = useDagSourceServiceGetDagSource(
- {
- fileToken: dag?.file_token ?? "",
- },
- undefined,
- { enabled: Boolean(dag?.file_token) },
- );
+ } = useDagSourceServiceGetDagSource({
+ dagId: dagId ?? "",
Review Comment:
is version_number also needed?
##########
airflow/api_connexion/endpoints/dag_source_endpoint.py:
##########
@@ -41,32 +41,42 @@
@mark_fastapi_migration_done
@security.requires_access_dag("GET", DagAccessEntity.CODE)
@provide_session
-def get_dag_source(*, file_token: str, session: Session = NEW_SESSION) -> Response:
- """Get source code using file token."""
- secret_key = current_app.config["SECRET_KEY"]
- auth_s = URLSafeSerializer(secret_key)
- try:
- path = auth_s.loads(file_token)
- dag_ids = session.query(DagModel.dag_id).filter(DagModel.fileloc == path).all()
- requests: Sequence[IsAuthorizedDagRequest] = [
- {
- "method": "GET",
- "details": DagDetails(id=dag_id[0]),
- }
- for dag_id in dag_ids
- ]
+def get_dag_source(
+ *,
+ dag_id: str,
+ version_number: int | None = None,
+ session: Session = NEW_SESSION,
+) -> Response:
+ """Get source code from DagCode."""
+ dag_version = DagVersion.get_version(dag_id, version_number, session=session)
+ if not dag_version:
+ raise NotFound(f"The source code of the DAG {dag_id}, version_number {version_number} was not found")
+ path = dag_version.dag_code.fileloc
+ dag_ids = session.scalars(select(DagModel.dag_id).where(DagModel.fileloc == path)).all()
+ requests: Sequence[IsAuthorizedDagRequest] = [
+ {
+ "method": "GET",
+ "details": DagDetails(id=dag_id[0]),
+ }
+ for dag_id in dag_ids
+ ]
- # Check if user has read access to all the DAGs defined in the file
- if not get_auth_manager().batch_is_authorized_dag(requests):
- raise PermissionDenied()
- dag_source = DagCode.code(path, session=session)
- except (BadSignature, FileNotFoundError):
- raise NotFound("Dag source not found")
+ # Check if user has read access to all the DAGs defined in the file
Review Comment:
Should we do the permission check before other operations?
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1847070331
##########
airflow/api_fastapi/core_api/routes/public/dag_sources.py:
##########
@@ -54,22 +53,24 @@
},
response_model=DAGSourceResponse,
)
-def get_dag_source(
- file_token: str,
+async def get_dag_source(
Review Comment:
Both are fine indeed
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1847070331
##########
airflow/api_fastapi/core_api/routes/public/dag_sources.py:
##########
@@ -54,22 +53,24 @@
},
response_model=DAGSourceResponse,
)
-def get_dag_source(
- file_token: str,
+async def get_dag_source(
Review Comment:
Yep or that
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1838414646
##########
airflow/models/dag_version.py:
##########
@@ -136,7 +136,8 @@ def get_latest_version(cls, dag_id: str, *, session: Session = NEW_SESSION) -> D
def get_version(
cls,
dag_id: str,
- version_number: int = 1,
+ version_number: int | None = None,
+ version_name: str | None = None,
Review Comment:
> From the UI perspective, how should users search for DAG version?
I'm not sure we really want users searching by DAG version, but it'd be more informational.
If we did, however, I think letting folks go by either would be fine. But it would be in the context of "dag runs for version x", moreso than searching for versions directly. None of that is planned at the moment afaik though.
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1836693192
##########
airflow/models/dag_version.py:
##########
@@ -136,7 +136,8 @@ def get_latest_version(cls, dag_id: str, *, session: Session = NEW_SESSION) -> D
def get_version(
cls,
dag_id: str,
- version_number: int = 1,
+ version_number: int | None = None,
+ version_name: str | None = None,
Review Comment:
The version number is incremented each time there's a structural change to the DAG, while the version name does not change. You can have all the DAGs with the same version name. I think for display, having `version_name-version_number` would be good. However, I'm wondering if we really need the version_name @jedcunningham.
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1834504950
##########
airflow/models/dag_version.py:
##########
@@ -136,7 +136,8 @@ def get_latest_version(cls, dag_id: str, *, session: Session = NEW_SESSION) -> D
def get_version(
cls,
dag_id: str,
- version_number: int = 1,
+ version_number: int | None = None,
+ version_name: str | None = None,
Review Comment:
What's the difference between the version number and name? Whatever is more human-readable is what the UI should use.
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1834504950
##########
airflow/models/dag_version.py:
##########
@@ -136,7 +136,8 @@ def get_latest_version(cls, dag_id: str, *, session: Session = NEW_SESSION) -> D
def get_version(
cls,
dag_id: str,
- version_number: int = 1,
+ version_number: int | None = None,
+ version_name: str | None = None,
Review Comment:
What's the difference between the version number and name? Is one better for display and another better for querying?
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy merged PR #43492:
URL: https://github.com/apache/airflow/pull/43492
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1843368421
##########
airflow/api_connexion/endpoints/dag_source_endpoint.py:
##########
@@ -41,32 +41,42 @@
@mark_fastapi_migration_done
@security.requires_access_dag("GET", DagAccessEntity.CODE)
@provide_session
-def get_dag_source(*, file_token: str, session: Session = NEW_SESSION) -> Response:
- """Get source code using file token."""
- secret_key = current_app.config["SECRET_KEY"]
- auth_s = URLSafeSerializer(secret_key)
- try:
- path = auth_s.loads(file_token)
- dag_ids = session.query(DagModel.dag_id).filter(DagModel.fileloc == path).all()
- requests: Sequence[IsAuthorizedDagRequest] = [
- {
- "method": "GET",
- "details": DagDetails(id=dag_id[0]),
- }
- for dag_id in dag_ids
- ]
+def get_dag_source(
+ *,
+ dag_id: str,
+ version_number: int | None = None,
+ session: Session = NEW_SESSION,
+) -> Response:
+ """Get source code from DagCode."""
+ dag_version = DagVersion.get_version(dag_id, version_number, session=session)
+ if not dag_version:
+ raise NotFound(f"The source code of the DAG {dag_id}, version_number {version_number} was not found")
+ path = dag_version.dag_code.fileloc
+ dag_ids = session.scalars(select(DagModel.dag_id).where(DagModel.fileloc == path)).all()
+ requests: Sequence[IsAuthorizedDagRequest] = [
+ {
+ "method": "GET",
+ "details": DagDetails(id=dag_id[0]),
+ }
+ for dag_id in dag_ids
+ ]
- # Check if user has read access to all the DAGs defined in the file
- if not get_auth_manager().batch_is_authorized_dag(requests):
- raise PermissionDenied()
- dag_source = DagCode.code(path, session=session)
- except (BadSignature, FileNotFoundError):
- raise NotFound("Dag source not found")
+ # Check if user has read access to all the DAGs defined in the file
Review Comment:
I think it was done in the right place. It's not only about this particular dag; we have to find other dags in the file and see if the user has permission to see them, too, before we can show the source code.
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1843378092
##########
airflow/api_connexion/endpoints/dag_source_endpoint.py:
##########
@@ -41,32 +41,42 @@
@mark_fastapi_migration_done
@security.requires_access_dag("GET", DagAccessEntity.CODE)
@provide_session
-def get_dag_source(*, file_token: str, session: Session = NEW_SESSION) -> Response:
- """Get source code using file token."""
- secret_key = current_app.config["SECRET_KEY"]
- auth_s = URLSafeSerializer(secret_key)
- try:
- path = auth_s.loads(file_token)
- dag_ids = session.query(DagModel.dag_id).filter(DagModel.fileloc == path).all()
- requests: Sequence[IsAuthorizedDagRequest] = [
- {
- "method": "GET",
- "details": DagDetails(id=dag_id[0]),
- }
- for dag_id in dag_ids
- ]
+def get_dag_source(
+ *,
+ dag_id: str,
+ version_number: int | None = None,
+ session: Session = NEW_SESSION,
+) -> Response:
+ """Get source code from DagCode."""
+ dag_version = DagVersion.get_version(dag_id, version_number, session=session)
+ if not dag_version:
+ raise NotFound(f"The source code of the DAG {dag_id}, version_number {version_number} was not found")
+ path = dag_version.dag_code.fileloc
+ dag_ids = session.scalars(select(DagModel.dag_id).where(DagModel.fileloc == path)).all()
+ requests: Sequence[IsAuthorizedDagRequest] = [
+ {
+ "method": "GET",
+ "details": DagDetails(id=dag_id[0]),
+ }
+ for dag_id in dag_ids
+ ]
- # Check if user has read access to all the DAGs defined in the file
- if not get_auth_manager().batch_is_authorized_dag(requests):
- raise PermissionDenied()
- dag_source = DagCode.code(path, session=session)
- except (BadSignature, FileNotFoundError):
- raise NotFound("Dag source not found")
+ # Check if user has read access to all the DAGs defined in the file
Review Comment:
ah, got it, makes sense
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1833465910
##########
airflow/models/dag_version.py:
##########
@@ -136,7 +136,8 @@ def get_latest_version(cls, dag_id: str, *, session: Session = NEW_SESSION) -> D
def get_version(
cls,
dag_id: str,
- version_number: int = 1,
+ version_number: int | None = None,
+ version_name: str | None = None,
Review Comment:
@jedcunningham, I remember we removed the version_name, but this seems necessary when using the API & UI to look up the code version. Let me know what you think.
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1846848727
##########
airflow/api_fastapi/core_api/routes/public/dag_sources.py:
##########
@@ -54,22 +53,24 @@
},
response_model=DAGSourceResponse,
)
-def get_dag_source(
- file_token: str,
+async def get_dag_source(
Review Comment:
route should not be defined `async`, more information here:
https://github.com/apache/airflow/pull/43797
--
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-65: Update dag source endpoint to support versioning [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #43492:
URL: https://github.com/apache/airflow/pull/43492#discussion_r1838394940
##########
airflow/models/dag_version.py:
##########
@@ -136,7 +136,8 @@ def get_latest_version(cls, dag_id: str, *, session: Session = NEW_SESSION) -> D
def get_version(
cls,
dag_id: str,
- version_number: int = 1,
+ version_number: int | None = None,
+ version_name: str | None = None,
Review Comment:
version_name is just a way for authors to add something that is meaningful for them. Strictly speaking we could do away with it completely.
--
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