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][Etcd] Support SSL In Etcd And Enhance Etcd In Helm #13924

Merged
merged 8 commits into from
Jun 14, 2023
Merged

[Improvement][Etcd] Support SSL In Etcd And Enhance Etcd In Helm #13924

merged 8 commits into from
Jun 14, 2023

Conversation

qingwli
Copy link
Member

@qingwli qingwli commented Apr 13, 2023 •
edited by SbloodyS
Loading

@SbloodyS SbloodyS added improvement make more easy to user or prompt friendly 3.2.0 for 3.2.0 version labels Apr 13, 2023
@SbloodyS SbloodyS added this to the 3.2.0 milestone Apr 13, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 13, 2023 •
edited
Loading

Codecov Report

Merging #13924 (5d810d1) into dev (c3de282) will decrease coverage by 0.40%.
The diff coverage is 43.07%.

❗ Current head 5d810d1 differs from pull request most recent head 0d55d95. Consider uploading reports for the commit 0d55d95 to get more accurate results

@@             Coverage Diff              @@
##                dev   #13924      +/-   ##
============================================
- Coverage     38.89%   38.50%   -0.40%     
- Complexity     4456     4561     +105     
============================================
  Files          1158     1239      +81     
  Lines         42429    43563    +1134     
  Branches       4776     4819      +43     
============================================
+ Hits          16504    16773     +269     
- Misses        24108    24933     +825     
- Partials       1817     1857      +40
Impacted Files Coverage Δ
...hinscheduler/plugin/alert/script/ScriptSender.java 72.22% <ø> (ø)
...inscheduler/alert/registry/AlertHeartbeatTask.java 0.00% <0.00%> (ø)
...nscheduler/api/configuration/AppConfiguration.java 93.93% <ø> (ø)
...inscheduler/api/controller/ExecutorController.java 17.94% <0.00%> (ø)
...lphinscheduler/api/controller/LoginController.java 51.72% <0.00%> (-16.46%) ⬇️
...er/api/controller/ProcessDefinitionController.java 58.62% <ø> (ø)
...uler/api/controller/ProcessInstanceController.java 65.30% <0.00%> (-4.26%) ⬇️
...nscheduler/api/controller/ResourcesController.java 53.70% <0.00%> (-1.02%) ⬇️
...nscheduler/api/controller/SchedulerController.java 82.60% <ø> (ø)
...eduler/api/dto/workflow/WorkflowCreateRequest.java 100.00% <ø> (ø)
... and 98 more

... and 213 files with indirect coverage changes

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

@sonarcloud
Copy link

sonarcloud bot commented Apr 13, 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 3 Code Smells

15.8% 15.8% Coverage
0.0% 0.0% Duplication

@zhongjiajie
Copy link
Member

@kezhenxu94 do you have a time to take a look this PR?

kezhenxu94
kezhenxu94 reviewed May 16, 2023
View reviewed changes
...ry-etcd/src/test/java/org/apache/dolphinscheduler/plugin/registry/etcd/EtcdRegistryTest.java Outdated
Comment on lines 145 to 146
@Disabled
@Test
Copy link
Member

Choose a reason for hiding this comment

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

What's the point to add a @Disabled test?

Copy link
Member Author

@qingwli qingwli May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This test method is used for the user to test connect etcd with configuration, user need to input their endpoints and SSL file.

Copy link
Member

Choose a reason for hiding this comment

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

This test method is used for the user to test connect etcd with configuration, user need to input their endpoints and SSL file.

Etcd has its own method to test connection, users don't even have the source codes, this test should be removed, and the comments should go into the doc

Copy link
Member Author

Choose a reason for hiding this comment

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

The comments has exist in readme.md

kezhenxu94
kezhenxu94 reviewed May 16, 2023
View reviewed changes
deploy/kubernetes/dolphinscheduler/templates/secret-external-etcd-ssl.yaml Outdated
Comment on lines 17 to 19
{{- if .Values.etcd.enabled }}
apiVersion: v1
kind: Secret
Copy link
Member

Choose a reason for hiding this comment

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

etcd being enabled doesn't indicate ssl is enabled too, so the condition {{- if .Values.etcd.enabled }} to create this Secret doesn't seem to make sense

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I will add a new var named ssl.enable

@qingwli
Copy link
Member Author

qingwli commented May 17, 2023

PTAL @kezhenxu94

1 similar comment
@qingwli
Copy link
Member Author

qingwli commented May 23, 2023

PTAL @kezhenxu94

@qingwli qingwli requested a review from kezhenxu94 May 30, 2023 06:14
kezhenxu94
kezhenxu94 previously approved these changes Jun 13, 2023
View reviewed changes
@sonarcloud
Copy link

sonarcloud bot commented Jun 13, 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 0 Code Smells

15.8% 15.8% Coverage
0.0% 0.0% Duplication

@zhongjiajie zhongjiajie merged commit 1c0dfbb into apache:dev Jun 14, 2023
@qingwli qingwli deleted the enhance-etcd branch June 14, 2023 05:42
zhongjiajie pushed a commit that referenced this pull request Jul 20, 2023
@qingwli @zhongjiajie
[Improvement][Etcd] Support SSL In Etcd And Enhance Etcd In Helm (#13924
a3e0594 
)

(cherry picked from commit 1c0dfbb)
@Gallardot Gallardot mentioned this pull request Apr 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kezhenxu94 kezhenxu94 kezhenxu94 approved these changes

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

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

@EricGao888 EricGao888 Awaiting requested review from EricGao888

Assignees

@qingwli qingwli

Labels
3.2.0 for 3.2.0 version backend improvement make more easy to user or prompt friendly
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.

[Improvement][Etcd] Support SSL In Etcd And Enhance Etcd In Helm
5 participants
@qingwli @codecov-commenter @zhongjiajie @kezhenxu94 @SbloodyS