-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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-6586][Server]add some ds process definition demo when init #11759
Conversation
1.add some ds process definition demo when init, to display what task type can run and make user easy to use ds. 2.need configure the JVM parameters (-Ddemo=true) to turn on the StandaloneServer service 3.need modify the tenant information in it
Hi @amaoisnb , you could add |
related: #6586 |
closes: apache#6586
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.
Please resolve conflicts.
BTW. It's better to use english in the commit message.
resolve the conflict
# Conflicts: # dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/controller/ProcessDefinitionControllerTest.java
...rc/test/java/org/apache/dolphinscheduler/api/controller/ProcessDefinitionControllerTest.java
Outdated
Show resolved
Hide resolved
dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/demo/DemoContants.java
Outdated
Show resolved
Hide resolved
I don’t think this is a good idea to couple the demo codes with the production code. At least, you should move the demo codes to the module dolphinscheduler-tools, and execute that tool after starting DS server. Mixing the demo codes and production code increases maintainability complexity |
+1 |
2 similar comments
+1 |
+1 |
...uler-tools/src/main/java/org/apache/dolphinscheduler/tools/datasource/CreateProcessDemo.java
Outdated
Show resolved
Hide resolved
create demo-tool
spotless
Codecov Report
@@ Coverage Diff @@
## dev #11759 +/- ##
============================================
- Coverage 39.61% 39.04% -0.57%
+ Complexity 4192 4183 -9
============================================
Files 1038 1043 +5
Lines 38843 39506 +663
Branches 4447 4544 +97
============================================
+ Hits 15387 15426 +39
- Misses 21712 22324 +612
- Partials 1744 1756 +12
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
fix bug
...inscheduler-tools/src/main/java/org/apache/dolphinscheduler/tools/demo/CreateDemoTenant.java
Show resolved
Hide resolved
...eduler-tools/src/main/java/org/apache/dolphinscheduler/tools/demo/ProcessDefinitionDemo.java
Outdated
Show resolved
Hide resolved
adjustments
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.
Previous comments are addressed, basically LGTM
dolphinscheduler-tools/src/main/java/org/apache/dolphinscheduler/tools/demo/ProxyResult.java
Show resolved
Hide resolved
...s/src/main/java/org/apache/dolphinscheduler/tools/demo/ProxyProcessDefinitionController.java
Outdated
Show resolved
Hide resolved
requestBodyMap.put("executionType", executionType); | ||
|
||
try { | ||
responseBody = OkHttpUtils.demoPost(url, token, requestBodyMap); |
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.
Ideally, I would prefer we should use RestTemplate
to perform http requests, but this looks OK to me if you want to refactor later.
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.
Then I'll look at it and try to make adjustments
some adjustments
some doc adjustments
The comments are not addressed but you click resolve conversation, please fix or reply if you think it's wrong. Don't simply mark it as resolved. |
I'm sorry I didn't reply to this |
make some adjustments
dolphinscheduler-tools/src/main/java/org/apache/dolphinscheduler/tools/demo/ProxyResult.java
Outdated
Show resolved
Hide resolved
make some adjustments
make some adjustments
SonarCloud Quality Gate failed. |
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.
LGTM if CI pass
…pache#11759) * [Feature-6586][Server]add some ds process definition demo when init 1.add some ds process definition demo when init, to display what task type can run and make user easy to use ds. 2.need configure the JVM parameters (-Ddemo=true) to turn on the StandaloneServer service 3.need modify the tenant information in it
…n init (apache#11759)" This reverts commit 72c985c.
[Feature-6586][Server]add some ds process definition demo when init
1.add some ds process definition demo when init, to display what task type can run and make user easy to
use ds.
2.need configure the JVM parameters (-Ddemo=true) to turn on the StandaloneServer service
3.need modify the tenant information in it