Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AIP-65: Update dag source endpoint to support versioning #43492

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ephraimbuddy
Copy link
Contributor

@ephraimbuddy ephraimbuddy commented Oct 29, 2024

Enhanced the DAG source endpoint to support version-based retrieval

Refactored the get_dag_source function to allow fetching specific versions of DAG source code using dag_id, version_name, and version_number parameters.

Replaced file_token with dag_id in endpoint paths and removed unnecessary token-based access.

Updated OpenAPI specifications and requested serializers to include new versioning parameters.

Modified API response schema to include dag_id, version_name, and version_number for improved version tracking.

Added/updated tests

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:db-migrations PRs with DB migration area:dev-tools area:Scheduler including HA (high availability) scheduler area:serialization area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues kind:documentation labels Oct 29, 2024
@ephraimbuddy ephraimbuddy force-pushed the version-api branch 2 times, most recently from bb2a352 to 3ec23a2 Compare October 30, 2024 06:21
@ephraimbuddy ephraimbuddy changed the title AIP-65: Make DAG source endpoint version aware AIP-65: Update dag source endpoint to support versioning Nov 7, 2024
@ephraimbuddy ephraimbuddy marked this pull request as ready for review November 7, 2024 22:42
@ephraimbuddy ephraimbuddy added the legacy api Whether legacy API changes should be allowed in PR label Nov 7, 2024
@@ -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,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the UI perspective, how should users search for DAG version? Should it be dag_id and number only?

Copy link
Contributor

@bbovenzi bbovenzi Nov 8, 2024

Choose a reason for hiding this comment

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

What's the difference between the version number and name? Is one better for display and another better for querying?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed the version_name altogether.

Enhanced the DAG source endpoint to support version-based retrieval

Refactored the get_dag_source function to allow fetching specific versions of DAG source code using dag_id, version_name, and version_number parameters.

Replaced file_token with dag_id in endpoint paths and removed unnecessary token-based access.

Updated OpenAPI specifications and requested serializers to include new versioning parameters.

Modified API response schema to include dag_id, version_name, and version_number for improved version tracking.

Added/updated tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API area:db-migrations PRs with DB migration area:dev-tools area:Scheduler including HA (high availability) scheduler area:serialization area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues kind:documentation legacy api Whether legacy API changes should be allowed in PR legacy ui Whether legacy UI change should be allowed in PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants