[Fix-17534][Service/Master] add global parameters and varpool from current workflow instance and add them to start params list of the trigger request of a sub workflow #17578
Conversation
|
Thanks for opening this pull request! Please check out our contributing guidelines. (https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md) |
kvermeulen
requested review from
SbloodyS,
caishunfeng and
ruanwenjun
as code owners
|
3.3.1 has been closed, please submit PR to dev. |
|
I tried rebasing this one on dev. Is this sufficient, or should I create an entire new PR? |
No need to create a new PR. |
kvermeulen
force-pushed
the
Fix-17534
branch
2 times, most recently
from
fae1b0b to
ff36be6
Compare
|
Fixed the 3 issues detected by sonarqube |
| .environmentCode(workflowInstance.getEnvironmentCode()) | ||
| // todo: transport varpool and local params | ||
| .startParamList(commandParam.getCommandParams()) | ||
| .startParamList(mergeParams(asList(commandParam.getCommandParams(), globalParamsOfParents))) |
There was a problem hiding this comment.
We should merge the current workflow instance's command params, global params, varpool as the subworkflow instance's command param is enough, we don't need to find the parant global params here, since the parent global params should already exist in command params.
There was a problem hiding this comment.
Sorry, I missed that indeed. Parent global params already exist in the command params. I'll update and remove the unnecessary code. Can you maybe verify if the order of merging the params is correct? Before I didn't include the varpool for instance.
I'll update my commit and force push on the PR. I don't know if this is best practice or if we'll loose trace of the previous change. However, I do like a code changes for a fix in a single commit. Makes it more concise maybe. But any advice here is also welcome.
|
… varpool from current workflow instance, together with the workflow instances command params to the sub workflow
kvermeulen
changed the title
|
Updated and corrected the title of the PR after receiving some review remarks from @ruanwenjun, who pointed out that only the global parameters and varpool of the current workflow instance, need to be passed when triggering a subflow. Global parameters from any parent workflow(s) will already be in the command params from the current workflow instance. (thanks for pointing out!) |
3 participants
Purpose of the pull request
Close #17534
Brief change log
When triggering a sub workflow, lookup all global parameters from the current, and any parent workflow instances and pass them along in the start param list of the trigger request of the sub workflow
Verify this pull request
This change added tests and can be verified as follows:
Pull Request Notice
Pull Request Notice
This is my very first pull-request, so bear with me on this one. :-)
A colleague of mine opened the original issue. This PR fixes these erroneous workflows for us, but I'm not entirely sure if this is the correct approach taken. I'll try and follow-up to make sure this PR meets all coding conventions and fulfills all requirements. I'll do my best on taking the time on improving the implementation on any pointers/advice given, if the implementation in the PR is not entirely correct or does not cover all use-cases, but I can't make any promises there.
Thank you for reviewing.