-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Ado-2898-execute-workflow-node-add-workflow-inputs-parameter #11887
Ado-2898-execute-workflow-node-add-workflow-inputs-parameter #11887
Conversation
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
packages/nodes-base/nodes/ExecuteWorkflow/methods/resourceMapping.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we need to bump the node version please do so now so we have it in the diff and don't forget about it later. LGTM otherwise.
Thanks @CharlieKolb , nice catch. we have already bump the node version with prev node changes. |
9dfd49c
to
7f436a9
Compare
…ado-2898-execute-workflow-node-add-workflow-inputs-parameter
@netroy this is the branch that I will base the work for workflowInputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some quick notes since I already wrote them before our call :D
packages/core/src/node-execution-context/workflow-inputs-context.ts
Outdated
Show resolved
Hide resolved
packages/core/src/node-execution-context/workflow-inputs-context.ts
Outdated
Show resolved
Hide resolved
packages/core/src/node-execution-context/workflow-inputs-context.ts
Outdated
Show resolved
Hide resolved
4bc4d04
to
dacd190
Compare
2f3b43b
to
c83bbcf
Compare
2156d79
to
44400a1
Compare
44400a1
to
f5b7ecd
Compare
packages/core/src/node-execution-context/local-load-options-context.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some initial nitpicks, will try the feature now
packages/nodes-base/nodes/ExecuteWorkflow/ExecuteWorkflowTrigger/ExecuteWorkflowTrigger.node.ts
Show resolved
Hide resolved
packages/editor-ui/src/components/ResourceMapper/ResourceMapper.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the actual expression mapping expected to work already? The value I provide is ignored and the inputData is still used directly, unless I'm holding it wrong :D Type version is 1.2, the latest.
…nto ado-2898-execute-workflow-node-add-workflow-inputs-parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes as github UI is bugged and my ancient approval is still repeated on every comment 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a minor comment. Will test the feature once more when I pull in the latest changes and leave further comments if I have any
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ready for the feature branch from my side, really nice work handling the complexity 🙌
Co-authored-by: Charlie Kolb <[email protected]>
613b9bd
to
5640c7b
Compare
6e21c80
to
9e8fddc
Compare
Summary
• Added new backend endpoint and load context for workflow parameters.
• Updated resource mapper component to load workflow input mapper.
• Extracted common logic from the execute workflow trigger node and reused it in the execute workflow node.
TODO:
Video: https://www.loom.com/share/06bdad96daa24e83809ebbe4191915d5
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/ADO-2950/add-resource-mapper-option-to-handle-workflow-inputs
https://linear.app/n8n/issue/ADO-2898/execute-workflow-node-add-workflow-inputs-parameter
Review / Merge checklist
release/backport
(if the PR is an urgent fix that needs to be backported)