fix(project): include trigger reply fields in version hash#1362
fix(project): include trigger reply fields in version hash#1362officialasishkumar wants to merge 1 commit intoOpenFn:mainfrom
Conversation
josephjclark
left a comment
josephjclark
left a comment
There was a problem hiding this comment.
Thank you @officialasishkumar! This is cool. Just a couple of tidy ups before I can merge it
.changeset/quiet-seals-battle.md
Outdated
| '@openfn/cli': patch | ||
| --- | ||
|
|
||
| Include `webhook_reply` and `cron_cursor_job_id` in workflow version hashes so CLI deploy stays aligned with Lightning. |
There was a problem hiding this comment.
You need to remove the "stay aligned with Lightnig bit" - this PR is just adding support for extra keys in the version hash, that's it.
| ` | ||
| ); | ||
|
|
||
| (wf1.steps[0] as any).openfn.webhook_reply = 'before_start'; |
There was a problem hiding this comment.
Instead of this awkward assignment, you can do this in the workflow generator - that's exactly what's it for!
generateWorkflow(
` @id wf1
t(type=webhook,webhook_reply=before_start)-x
`
Please also make sure you're generating the absolute minimal keys on the workflow so that it doesn't distract the test code. We shouldn't need an expression on x for example
There was a problem hiding this comment.
Moved this into the workflow generator and trimmed the test setup in f021d98.
| `@name wf-1 | ||
| @id workflow-id | ||
| t(type=cron,cron_expression="1")-a | ||
| a-b |
There was a problem hiding this comment.
You shouldn't need the a-b nodes here at all right? Better to clean that up and keep the tests simple
There was a problem hiding this comment.
Removed the extra nodes and kept the test minimal in f021d98.
| const [trigger1, jobA1] = wf1.steps as any[]; | ||
| const [trigger2, , jobB2] = wf2.steps as any[]; | ||
|
|
||
| trigger1.openfn.cron_cursor_job_id = jobA1.openfn.uuid; |
There was a problem hiding this comment.
Again you can do this straight in workflow generation. It doesn't need to be logically consistent either- you can just set the value to x or y, rather than digging out the actual uuids like this
There was a problem hiding this comment.
Changed this to literal x and y values in the generated workflow in f021d98.
|
Oh and @officialasishkumar we like contributors to disclose AI usage in some way so that everyone knows where we stand. We have a checkbox for this in the PR template. Can you update the PR description to at least say "I have used AI to generate this PR"? I've updated the PR description to remove the weird validation stuff. It's enough in this case that tests are passing in CI |
|
Hi @josephjclark , yes, i have used AI in research and understanding the codebase |
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
officialasishkumar
force-pushed
the
fix/version-hash-trigger-keys
branch
from
242e36c to
f021d98
Compare
2 participants
Description
This keeps the project-side workflow version hash aligned with Lightning by including
webhook_replyandcron_cursor_job_idin the trigger field list used bygenerateHash().Without this, the CLI can compute a different hash from Lightning for the same workflow after those trigger fields change.
Changes
webhook_replyandcron_cursor_job_idinpackages/project/src/util/version.tspackages/project/test/util/version-workflow.test.tsCompanion to OpenFn/lightning#4599.
AI Usage
Used only for research.