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

[Improvement][Helm] Refactor configmap control in master #15935

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

pegasas
Copy link
Contributor

@pegasas pegasas commented Apr 28, 2024

Purpose of the pull request

fix: #15934

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

@ruanwenjun
Copy link
Member

@Gallardot PTAL

@ruanwenjun ruanwenjun added the improvement make more easy to user or prompt friendly label Apr 29, 2024
@ruanwenjun ruanwenjun added this to the 3.2.2 milestone Apr 29, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 40.46%. Comparing base (680d2e0) to head (9dd55b2).
Report is 1 commits behind head on dev.

❗ Current head 9dd55b2 differs from pull request most recent head 02e3cc8. Consider uploading reports for the commit 02e3cc8 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##                dev   #15935   +/-   ##
=========================================
  Coverage     40.45%   40.46%           
+ Complexity     5200     5196    -4     
=========================================
  Files          1378     1378           
  Lines         46084    46084           
  Branches       4922     4923    +1     
=========================================
+ Hits          18645    18649    +4     
+ Misses        25513    25508    -5     
- Partials       1926     1927    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- name: config-volume
mountPath: /opt/dolphinscheduler/conf/common.properties
subPath: common.properties
{{- include "dolphinscheduler.etcd.ssl.volumeMount" . | nindent 12 }}
Copy link
Member

Choose a reason for hiding this comment

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

Why delete the ssl volume of etcd?

ref: #13924
@qingwli PTAL.

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 seems this is also needed in api,
so I kept this and adjust the order to make it clear to read.

@@ -109,11 +109,9 @@ spec:
volumeMounts:
- mountPath: "/opt/dolphinscheduler/logs"
name: {{ include "dolphinscheduler.fullname" . }}-master
{{- include "dolphinscheduler.sharedStorage.volumeMount" . | nindent 12 }}
Copy link
Member

Choose a reason for hiding this comment

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

That's fine with me. Why not remove this from api-server as well.

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 seems this config is needed,
I adjust the order and fix some config in worker

@pegasas pegasas changed the title [Improvement][Helm] Remove useless configmap control in master [Improvement][Helm] Refactor configmap control in master Apr 30, 2024
Comment on lines -136 to -140
{{- else }}
- name: {{ include "dolphinscheduler.fullname" . }}-worker-data
emptyDir: {}
- name: {{ include "dolphinscheduler.fullname" . }}-worker-logs
emptyDir: {}
Copy link
Member

Choose a reason for hiding this comment

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

Seems like these lines shouldn't be removed?

Copy link
Member

Choose a reason for hiding this comment

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

Seems like these lines shouldn't be removed?

+1

- name: config-volume
configMap:
name: {{ include "dolphinscheduler.fullname" . }}-configs
{{- include "dolphinscheduler.sharedStorage.volume" . | nindent 8 }}
Copy link
Member

Choose a reason for hiding this comment

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

Just change from line115 to line 128?

Comment on lines -136 to -140
{{- else }}
- name: {{ include "dolphinscheduler.fullname" . }}-worker-data
emptyDir: {}
- name: {{ include "dolphinscheduler.fullname" . }}-worker-logs
emptyDir: {}
Copy link
Member

Choose a reason for hiding this comment

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

Seems like these lines shouldn't be removed?

+1

Copy link

sonarcloud bot commented May 14, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement make more easy to user or prompt friendly kubernetes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement][Helm] Refactor configmap control in master
7 participants