Add the option to select bundle version parameter on dag run trigger endpoint#61550
Add the option to select bundle version parameter on dag run trigger endpoint#61550itayweb wants to merge 64 commits into
Conversation
itayweb
requested review from
ashb,
bolkedebruin,
bugraoz93,
choo121600,
ephraimbuddy,
jason810496,
pierrejeambrun,
rawwar and
shubhamraj-git
as code owners
itayweb
force-pushed
the
feat-bundle-version-support-dagrun
branch
from
5b3753a to
eea1f7d
Compare
itayweb
requested review from
bbovenzi,
guan404ming,
ryanahamilton and
vatsrahul1001
as code owners
itayweb
force-pushed
the
feat-bundle-version-support-dagrun
branch
from
eea1f7d to
a862cd3
Compare
itayweb
force-pushed
the
feat-bundle-version-support-dagrun
branch
from
a862cd3 to
d47dcd4
Compare
itayweb
force-pushed
the
feat-bundle-version-support-dagrun
branch
2 times, most recently
from
4aad19c to
a87673a
Compare
jason810496
left a comment
jason810496
left a comment
There was a problem hiding this comment.
Thanks for the improvement!
Please add the corresponding tests when you are convenient, thanks.
pierrejeambrun
left a comment
pierrejeambrun
left a comment
There was a problem hiding this comment.
Can you add the requested unit tests please so we can merge this.
|
I've added the unit test, let me know what do you think 😄 sorry for the delay |
|
Quickest fix: git fetch upstream main && git rebase upstream/main
rm uv.lock && uv lock
git add uv.lock && git rebase --continue
git push --force-with-leaseAutomated nudge — ignore if you're not ready to rebase. This comment is updated in place on future |
|
Overall looks great. @itayweb could you help resolve the conflict, thanks! |
Done 👍 |
|
@itayweb Can you look at failing test? |
pierrejeambrun
left a comment
pierrejeambrun
left a comment
There was a problem hiding this comment.
Thanks for your continuous effort on this. I believe we are getting close.
LGTM, just a few small adjusments
pierrejeambrun
changed the title
|
Updated the description to follow our commit name policy. (cf Ash comment) |
…ate introduction timeline
| @@ -653,7 +656,25 @@ def create_dagrun( | |||
| self.validate_partition_key(partition_key) | |||
There was a problem hiding this comment.
create_dagrun() still validates partition_key against self before resolving the requested bundle version. In the API trigger path self is the latest Dag, so bundle_version="v1" can still fail for a partitioned v1 if latest v2 is not partitioned. Please resolve the requested Dag version first and validate against that Dag.
| f"Dag with dag_id: '{dag_id}' has import errors and cannot be triggered", | ||
| ) | ||
|
|
||
| if dm.allowed_run_types is not None and DagRunType.MANUAL not in dm.allowed_run_types: |
There was a problem hiding this comment.
Are we intentionally using the latest Dag/DagModel for allowed_run_types even when bundle_version points at an older Dag version? The rest of this path now derives params/timetable/tasks from the requested version, so this can reject a run v1 allows or allow a run v1 disallows. If current policy should govern triggerability, please add a test for that; otherwise this should move to context_dag.allowed_run_types after version resolution.
There was a problem hiding this comment.
core_api/routes/public/assets.py line 449 has the same issue
A new feature in the dag run REST API, adding the option to add bundle version parameter to trigger specific version.
Wasn't sure how can I add to this a unit test due to need a git repo/s3 bucket, let me know if needed
Was generative AI tooling used to co-author this PR?
Generated-by: [Cursor] following the guidelines