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.

Sign up for GitHub

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

Jump to bottom

[Improvement-12536][k8s] Support the command for the container in k8s task plugin #12538

Merged
merged 1 commit into from
Oct 31, 2022
Merged

[Improvement-12536][k8s] Support the command for the container in k8s task plugin #12538

merged 1 commit into from
Oct 31, 2022

Conversation

rickchengx
Copy link
Contributor

Purpose of the pull request

Brief change log

  • Support the command for the container in k8s task plugin
  • The user can set the startup command of the container
  • If the command is not set, the default startup command of the image is executed.

Verify this pull request

截屏2022-10-26 14 06 39

  • The spec for the container:

截屏2022-10-26 14 14 41

  • The ouput for the container:

截屏2022-10-26 14 05 15

@github-actions github-actions bot added backend UI ui and front end related labels Oct 26, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2022 •
edited
Loading

Codecov Report

Merging #12538 (7413487) into dev (e6da1cc) will increase coverage by 0.02%.
The diff coverage is 75.00%.

@@             Coverage Diff              @@
##                dev   #12538      +/-   ##
============================================
+ Coverage     39.05%   39.07%   +0.02%     
+ Complexity     4190     4184       -6     
============================================
  Files          1044     1043       -1     
  Lines         39486    39460      -26     
  Branches       4537     4536       -1     
============================================
- Hits          15422    15420       -2     
+ Misses        22313    22284      -29     
- Partials       1751     1756       +5
Impacted Files Coverage Δ
...olphinscheduler/plugin/task/api/TaskConstants.java 0.00% <ø> (ø)
.../plugin/task/api/parameters/K8sTaskParameters.java 0.00% <ø> (ø)
...uler/plugin/task/api/k8s/impl/K8sTaskExecutor.java 36.52% <71.42%> (+1.52%) ⬆️
...ache/dolphinscheduler/plugin/task/k8s/K8sTask.java 85.71% <100.00%> (+0.71%) ⬆️
...ler/plugin/task/api/k8s/K8sTaskMainParameters.java
...e/dolphinscheduler/remote/NettyRemotingClient.java 52.08% <0.00%> (+0.69%) ⬆️
...org/apache/dolphinscheduler/remote/utils/Host.java 44.68% <0.00%> (+2.12%) ⬆️
...erver/master/processor/queue/TaskEventService.java 80.35% <0.00%> (+10.71%) ⬆️

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

caishunfeng
caishunfeng reviewed Oct 26, 2022
View reviewed changes
.../src/main/java/org/apache/dolphinscheduler/plugin/task/api/parameters/K8sTaskParameters.java Outdated
Comment on lines 70 to 77
public String getCommand() {
return command;
}

public void setCommand(String command) {
this.command = command;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

remove these with @Data

@caishunfeng caishunfeng added this to the 3.2.0 milestone Oct 27, 2022
@caishunfeng caishunfeng added the improvement make more easy to user or prompt friendly label Oct 27, 2022
@rickchengx
Copy link
Contributor Author

remove these with @Data

Hi, @caishunfeng , thanks for the review. I have used @Data in K8sTaskParameters and K8sTaskMainParameters

caishunfeng
caishunfeng reviewed Oct 27, 2022
View reviewed changes
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.

Please update the doc of k8s task too.

.../src/main/java/org/apache/dolphinscheduler/plugin/task/api/parameters/K8sTaskParameters.java Outdated Show resolved Hide resolved
...api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/k8s/K8sTaskMainParameters.java Outdated Show resolved Hide resolved
caishunfeng
caishunfeng previously approved these changes Oct 31, 2022
View reviewed changes
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, thanks @rickchengx

Amy0104
Amy0104 reviewed Oct 31, 2022
View reviewed changes
dolphinscheduler-ui/src/views/projects/task/components/node/fields/use-k8s.ts
validate: {
trigger: ['input', 'blur'],
message: t('project.node.command_tips')
}
Copy link
Member

Choose a reason for hiding this comment

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

what is this to validate? required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @Amy0104 Thanks for the comment.

Actually, this validate is copied from the above code. (I am not familiar with front-end code)

截屏2022-10-31 14 29 12

Because this field command is not necessary, user can leave it blank.

So we just remove this validate code as below, right?

image

Copy link
Member

Choose a reason for hiding this comment

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

It's better to remove it, if u don't need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's better to remove it, if u don't need it.

ok, i got it. Thanks again for your comment~

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

85.7% 85.7% Coverage
0.0% 0.0% Duplication

Amy0104
Amy0104 approved these changes Oct 31, 2022
View reviewed changes
Copy link
Member

@Amy0104 Amy0104 left a comment

Choose a reason for hiding this comment

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

The front end part LGTM.

@Amy0104 Amy0104 merged commit 3c31ddf into apache:dev Oct 31, 2022
hstdream pushed a commit to hstdream/dolphinscheduler that referenced this pull request Nov 2, 2022
@rickchengx
[Improvement-12536][k8s] Support the command for the container in k8s…
b37bf64 
… task plugin (apache#12538)
hstdream pushed a commit to hstdream/dolphinscheduler that referenced this pull request Nov 2, 2022
Revert "[Improvement-12536][k8s] Support the command for the containe…
1469c3a 
…r in k8s task plugin (apache#12538)"

This reverts commit b37bf64.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Amy0104 Amy0104 Amy0104 approved these changes

@caishunfeng caishunfeng caishunfeng left review comments

@songjianet songjianet Awaiting requested review from songjianet songjianet is a code owner

@SbloodyS SbloodyS Awaiting requested review from SbloodyS SbloodyS is a code owner

@zhuangchong zhuangchong Awaiting requested review from zhuangchong

@zhongjiajie zhongjiajie Awaiting requested review from zhongjiajie

@Tianqi-Dotes Tianqi-Dotes Awaiting requested review from Tianqi-Dotes

@EricGao888 EricGao888 Awaiting requested review from EricGao888 EricGao888 is a code owner

Assignees

@rickchengx rickchengx

Labels
backend document improvement make more easy to user or prompt friendly UI ui and front end related
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.

[Improvement][k8s] Support the command for the container in k8s task plugin
4 participants
@rickchengx @codecov-commenter @caishunfeng @Amy0104