Skip to content

Conversation

@rawwar
Copy link
Contributor

@rawwar rawwar commented Feb 27, 2025

related to #42360

Pending:

  • tests for 403

jedcunningham and others added 3 commits February 25, 2025 20:38
@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. labels Feb 27, 2025
@rawwar rawwar force-pushed the kalyan/AIP-84/auth/asset branch from 8371e29 to 44e9883 Compare February 27, 2025 07:27
@rawwar rawwar marked this pull request as ready for review February 27, 2025 18:26
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

thanks

@rawwar
Copy link
Contributor Author

rawwar commented Mar 1, 2025

@pierrejeambrun , We now have an Asset Alias. Can we consider this as an Asset Access Entity? Similar to how Dag runs is a Dag Access Entity.

Or should these be considered a separate entity and create AssetAliasDetail? As alias will have its own id and use invokes API with /assets/aliases/{asset_alias_id},. We will not get any info about the asset_id here.

@rawwar rawwar requested review from eladkal and o-nikolas as code owners March 1, 2025 10:27
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Overall looks good.

A few small suggestions.

@rawwar
Copy link
Contributor Author

rawwar commented Mar 3, 2025

@pierrejeambrun , We now have an Asset Alias. Can we consider this as an Asset Access Entity? Similar to how Dag runs is a Dag Access Entity.

Or should these be considered a separate entity and create AssetAliasDetail? As alias will have its own id and use invokes API with /assets/aliases/{asset_alias_id},. We will not get any info about the asset_id here.

I've separated out asset alias endpoints to a separate PR - #47241

@jedcunningham jedcunningham added the AIP-84 Modern Rest API label Mar 4, 2025
@pierrejeambrun
Copy link
Member

There's one comment un-addressed, otherwise looks good.

@pierrejeambrun pierrejeambrun added the full tests needed We need to run full set of tests for this PR to merge label Mar 5, 2025
@pierrejeambrun pierrejeambrun reopened this Mar 5, 2025
@rawwar

This comment was marked as outdated.

@pierrejeambrun pierrejeambrun merged commit 50ff8b5 into apache:main Mar 6, 2025
89 checks passed
@Lee-W Lee-W mentioned this pull request Mar 6, 2025
47 tasks
nailo2c pushed a commit to nailo2c/airflow that referenced this pull request Apr 4, 2025
* Fix breeze api-server - missed it during config move

When I renamed the command to start the fastapi api server, I missed
updating the breeze config to reflect the new config section.

* init asset

* update rest

* add commit session after creating dags

* fix issue with time_machine

* session commit in make_dag_with_multiple_versions

* fix ci failure

* add unauthorized user tests

* fix

* fix

* move dag.sync_to_db to end

* pr feedback

* fix

* replace uri with id in AssetDetails

* remove asset alias tests

* remove unused session

* add newsfragment

* rename newsfragment

* use lambda

---------

Co-authored-by: Jed Cunningham <jedcunningham@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AIP-84 Modern Rest API area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. full tests needed We need to run full set of tests for this PR to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants