Skip to content
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

[Feature] Add project name/workflow definition name/instance id/task name to the built-in variables #14099

Merged
merged 6 commits into from
May 25, 2023

Conversation

haibingtown
Copy link
Contributor

@haibingtown haibingtown commented May 14, 2023

Purpose of the pull request

close #14007

Brief change log

Add project name/workflow definition name/instance id/task name to the built-in variables

Verify this pull request

This pull request is code cleanup without any test coverage.

Copy link
Member

@zhongjiajie zhongjiajie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@haibingtown
Copy link
Contributor Author

@zhongjiajie The comments are spot on, they have been resolved

@calvinjiang calvinjiang added this to the 3.2.0 milestone May 16, 2023
Copy link
Contributor

@calvinjiang calvinjiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look at those tips that I offer.

@zhuangchong
Copy link
Contributor

Please execute the spotless command mvn spotless:apply in the local project to format the document style. @haibingtown

@zhuangchong zhuangchong added the 3.2.0 for 3.2.0 version label May 17, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 17, 2023

Codecov Report

Merging #14099 (79707ab) into dev (8315279) will decrease coverage by 0.08%.
The diff coverage is 44.64%.

❗ Current head 79707ab differs from pull request most recent head 59490f3. Consider uploading reports for the commit 59490f3 to get more accurate results

@@             Coverage Diff              @@
##                dev   #14099      +/-   ##
============================================
- Coverage     38.41%   38.33%   -0.08%     
- Complexity     4461     4470       +9     
============================================
  Files          1223     1229       +6     
  Lines         42549    42887     +338     
  Branches       4716     4762      +46     
============================================
+ Hits          16345    16442      +97     
- Misses        24403    24622     +219     
- Partials       1801     1823      +22     
Impacted Files Coverage Δ
...nscheduler/api/controller/SchedulerController.java 82.60% <ø> (ø)
...heduler/api/service/impl/SchedulerServiceImpl.java 30.13% <0.00%> (-0.09%) ⬇️
...cheduler/common/log/remote/S3RemoteLogHandler.java 0.00% <0.00%> (ø)
.../apache/dolphinscheduler/common/utils/OSUtils.java 29.55% <0.00%> (-0.38%) ⬇️
.../master/processor/MasterTaskDispatchProcessor.java 0.00% <0.00%> (ø)
...rver/master/processor/MasterTaskKillProcessor.java 0.00% <0.00%> (ø)
...ver/master/processor/MasterTaskPauseProcessor.java 0.00% <0.00%> (ø)
...asterDelayTaskExecuteRunnableDelayQueueLooper.java 0.00% <0.00%> (ø)
.../server/master/runner/WorkflowExecuteRunnable.java 10.38% <0.00%> (+0.04%) ⬆️
...unner/execute/AsyncMasterTaskDelayQueueLooper.java 0.00% <0.00%> (ø)
... and 22 more

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zhuangchong
Copy link
Contributor

@haibingtown
At present, The CI reports an error: the front-end module build is abnormal, but this pr does not change the front-end code. Can you merge the latest code of the dev branch to try to see if this problem is solved?

caishunfeng
caishunfeng previously approved these changes May 18, 2023
Copy link
Contributor

@caishunfeng caishunfeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@haibingtown
Copy link
Contributor Author

@zhuangchong Still having issues with CI, what do I need to do

@zhuangchong
Copy link
Contributor

@zhuangchong Still having issues with CI, what do I need to do

@haibingtown The dev branch has just solved this problem, can you merge it once?

@haibingtown
Copy link
Contributor Author

@zhuangchong ok, done

@haibingtown
Copy link
Contributor Author

@zhuangchong @calvinjiang @caishunfeng i guess it seems that this CI still needs a brother to help

SbloodyS
SbloodyS previously approved these changes May 19, 2023
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@SbloodyS
Copy link
Member

@SbloodyS I don't think this failure has anything to do with my code, or can you give me some pointers on where to look for this problem. At the same time, it seems that the coverage rate has not passed, who can help to see。

The workflow instance execution failed. You can download this video to simulate the operation locally.
https://github.com/apache/dolphinscheduler/suites/12985499794/artifacts/704246970

@haibingtown
Copy link
Contributor Author

@SbloodyS @zhuangchong @calvinjiang I have cherry-pick my commits, can you guys trigger CI again

@zhongjiajie
Copy link
Member

@SbloodyS I don't think this failure has anything to do with my code, or can you give me some pointers on where to look for this problem. At the same time, it seems that the coverage rate has not passed, who can help to see。

The workflow instance execution failed. You can download this video to simulate the operation locally. https://github.com/apache/dolphinscheduler/suites/12985499794/artifacts/704246970

FYI, we only have one day for our docker images, so if you may sure your code is correct you can re-run all tests to avoid docker image expired

@zhongjiajie
Copy link
Member

approval the ci run

@zhongjiajie
Copy link
Member

restart the failed ci

calvinjiang
calvinjiang previously approved these changes May 24, 2023
@calvinjiang
Copy link
Contributor

calvinjiang commented May 24, 2023

@SbloodyS @zhuangchong @calvinjiang I have cherry-pick my commits, can you guys trigger CI again
@haibingtown
I just restarted the E2E test. But it seems like that there is an error with running workflow. You can check the class 'WorkflowE2ETest' and the test case 'testRunWorkflow'. You'll find that the whole test class includes three test cases in order. Before running the test case 'testRunWorkflow' the test case 'testCreateWorkflow' and 'testCreateSubWorkflow' needs to be ran. As you can see, the two test cases use some parameters that might cause the error. You should simulate these settings in your workflow and see if there is the same error.

The error log is as follows:
image

image

@zhuangchong
Copy link
Contributor

@haibingtown
I fixed the E2E exception, the cause of the exception is that timeZone is empty when manually triggering the workflow.

private Map<String, String> setBuiltInParamsMap(@NonNull TaskInstance taskInstance,
@NonNull String timeZone) {
CommandType commandType = taskInstance.getProcessInstance().getCmdTypeIfComplement();

@zhuangchong
Copy link
Contributor

@haibingtown

Thank you for your valuable contribution. Your PR is excellent! I would like to establish a deeper communication with you. You can reach me via email or add me on WeChat (KerwinZhuang). When contacting me, please provide your identity for reference. If you encounter any difficulties with DolphinScheduler, I am more than happy to assist you and help you become familiar with the platform.

@sonarcloud
Copy link

sonarcloud bot commented May 25, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 15 Code Smells

32.0% 32.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@calvinjiang calvinjiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@calvinjiang calvinjiang merged commit af7da06 into apache:dev May 25, 2023
@Gallardot
Copy link
Member

ly109974 pushed a commit to ly109974/dolphinscheduler that referenced this pull request Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.2.0 for 3.2.0 version backend document feature new feature first time contributor First-time contributor miss:docs missing documents in PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add project id/workflow definition id/instance id to the built-in variables
9 participants