Uh oh!
There was an error while loading. Please reload this page.
AIP-103: Passing AssetStateStoreAccessors through to BaseEventTrigger - #67839
Conversation
… context['asset_state']
jroachgolf84
commented
Jun 1, 2026
cc: @cmarteepants |
jroachgolf84
commented
Jun 1, 2026
cc: @vincbeck and @amoghrajesh, I closed the previous PR (#66595) after renaming the branch. We'll use this one instead. |
vincbeck
left a comment
There was a problem hiding this comment.
One nit but overall I like it
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
jroachgolf84
commented
Jun 4, 2026
Unless I get anymore feedback, I'm going to start to write tests for this. |
AssetStateAccessors through to BaseEventTriggerAssetStateStoreAccessors through to BaseEventTriggerjroachgolf84
commented
Jun 12, 2026
Going to wait for @amoghrajesh to merge #68438 to make changes. |
amoghrajesh
commented
Jun 18, 2026
Right, i am looking at this one now. |
amoghrajesh
left a comment
There was a problem hiding this comment.
Thanks for pushing through this, LGTM.
Docs PR will follow?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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 |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
jroachgolf84
commented
Jun 19, 2026
@amoghrajesh - good to merge? |
amoghrajesh
commented
Jun 22, 2026
Rebasing this one, to ensure CI stays green. |
Uh oh!
There was an error while loading. Please reload this page.
Description
To ensure that classes inheriting from the
BaseEventTrigger(typically used for Asset watching) are "Asset-aware", the work done in AIP-103 has been wired into theBaseEventTriggerclass.#65103 drafted an approach that "flipped" the definition of the
AssetandAssetWatcher. However, for good reason, the approach was challenged. After conversation, the goal became to "pass" theAssetStateAccessorsthrough to theBaseEventTriggerwith some runtime magic.With this model, defining an
AssetandAssetWatcherremains the same as before.The two PR's below were closed in favor of this PR:
aip-93-scoping: AddingAssetWatcher.assetfunctionality #65103name/uriandAssetStateAccessorthrough toBaseEventTrigger#66595Testing
No unit tests have been written for this logic yet. However, testing has been performed E2E locally with
breeze. The trigger authored below is what was used for testing.Follow Up Work
This PR only injects the accessor, but does not yet wire the get/set logic. In order for the
AssetStateStoreAccessor.get()/.set()to be used from within theBaseEventTrigger, additional changes in_handle_request()will need to be made. That will be done in a follow-up PR.