[Improvement][Helm] Make configmap of api/master/worker/alert configuration #16058

pegasas · 2024-05-23T06:59:11Z

…ration

Purpose of the pull request

resolve: #16028

Brief change log

we found that it will bring multi application.yaml maintainence cost after involving new configmap into helm chart.
After discussion we decide to add a flag and default false,
then we can keep application.yaml in api/master/worker/alert clean.

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

Gallardot

Good job, LGTM.

Radeity · 2024-05-23T14:59:20Z

Then datasource.profile added in #15922 is useless?

pegasas · 2024-05-27T01:28:49Z

Then datasource.profile added in #15922 is useless?

We do not add datasource.profile added in #15922 for that PR.
We can still implement datasource.profile in current design by setting variable.

codecov-commenter · 2024-05-27T03:51:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 40.72%. Comparing base (61a6bd0) to head (13905ca).

❗ Current head 13905ca differs from pull request most recent head 7b5f248

Please upload reports for the commit 7b5f248 to get more accurate results.

Additional details and impacted files

@@             Coverage Diff              @@
##                dev   #16058      +/-   ##
============================================
+ Coverage     40.70%   40.72%   +0.01%     
- Complexity     5245     5247       +2     
============================================
  Files          1385     1385              
  Lines         46109    46109              
  Branches       4923     4923              
============================================
+ Hits          18768    18776       +8     
+ Misses        25414    25407       -7     
+ Partials       1927     1926       -1

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

Radeity · 2024-05-27T04:10:51Z

Hi @pegasas , please fix CI.

Gallardot · 2024-05-27T04:14:03Z

deploy/kubernetes/dolphinscheduler/values.yaml

@@ -449,7 +449,17 @@ master:
  #   requests:
  #     memory: "2Gi"
  #     cpu: "500m"
-
+  enableCustomizedConfig: false
+  customizedConfig: { }


New configuration items need to be commented

Add comments link to origin application.yaml path.

Please refer to helm doc to write comments so that helm doc can automatically generate documentation

Sure. Done.

Gallardot · 2024-05-27T04:14:44Z

deploy/kubernetes/dolphinscheduler/templates/configmap-dolphinscheduler-api.yaml

+  {{ $path }}: |
+{{ $config | indent 4 -}}
+{{- end -}}
+{{- end -}}


need a new line at the end of the file

Radeity · 2024-05-28T08:19:37Z

Help check this? cc @Gallardot

deploy/kubernetes/dolphinscheduler/templates/statefulset-dolphinscheduler-worker.yaml

…ration Update deploy/kubernetes/dolphinscheduler/templates/statefulset-dolphinscheduler-worker.yaml Co-authored-by: Gallardot <gallardot@apache.org>

deploy/kubernetes/dolphinscheduler/values.yaml

Co-authored-by: Wenjun Ruan <wenjun@apache.org>

pegasas · 2024-06-11T08:28:57Z

hi, team, would you like to review the PR when anytime you are available? Thanks

rickchengx

LGTM, this could avoid multiple application.yaml

Fixed.

SbloodyS

+1

sonarcloud · 2024-06-21T08:35:16Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

sonarcloud · 2024-06-21T08:35:17Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

…ration (apache#16058) Update deploy/kubernetes/dolphinscheduler/templates/statefulset-dolphinscheduler-worker.yaml * Update deploy/kubernetes/dolphinscheduler/values.yaml

* [Fix-16174] Incorrect cluster installation guide. (#16208) * [Fix][CI] fix the ci error of Values.datasource.profile (#16031) * [Improvement][Helm] Make configmap of api/master/worker/alert configuration (#16058) Update deploy/kubernetes/dolphinscheduler/templates/statefulset-dolphinscheduler-worker.yaml * Update deploy/kubernetes/dolphinscheduler/values.yaml * [helm] remove appversion from labels (#16066)

pegasas requested a review from caishunfeng as a code owner May 23, 2024 06:59

Gallardot approved these changes May 23, 2024

View reviewed changes

Radeity previously approved these changes May 27, 2024

View reviewed changes

Gallardot reviewed May 27, 2024

View reviewed changes

pegasas dismissed Radeity’s stale review via b9646ee May 27, 2024 06:53

pegasas force-pushed the issues/16028 branch 3 times, most recently from 7b33d3e to 9286014 Compare May 27, 2024 07:22

Gallardot approved these changes May 28, 2024

View reviewed changes

Gallardot requested changes May 29, 2024

View reviewed changes

deploy/kubernetes/dolphinscheduler/templates/statefulset-dolphinscheduler-worker.yaml Outdated Show resolved Hide resolved

[Improvement][Helm] Make configmap of api/master/worker/alert configu…

a2b328a

…ration Update deploy/kubernetes/dolphinscheduler/templates/statefulset-dolphinscheduler-worker.yaml Co-authored-by: Gallardot <gallardot@apache.org>

pegasas force-pushed the issues/16028 branch from 2038f17 to a2b328a Compare May 29, 2024 15:55

Gallardot approved these changes May 30, 2024

View reviewed changes

Radeity previously approved these changes May 30, 2024

View reviewed changes

Merge branch 'dev' into issues/16028

cd94f96

Radeity assigned pegasas May 30, 2024

Merge branch 'dev' into issues/16028

2b2b785

Radeity added improvement make more easy to user or prompt friendly ready-to-merge 3.3.0 labels May 30, 2024

Radeity added this to the 3.3.0 milestone May 30, 2024

Radeity requested review from ruanwenjun, rickchengx and EricGao888 May 30, 2024 11:55

ruanwenjun changed the title ~~[Improvement][Helm] Make configmap of api/master/worker/alert configu…~~ [Improvement][Helm] Make configmap of api/master/worker/alert configuration May 30, 2024

ruanwenjun previously requested changes May 30, 2024

View reviewed changes

deploy/kubernetes/dolphinscheduler/values.yaml Outdated Show resolved Hide resolved

Update deploy/kubernetes/dolphinscheduler/values.yaml

ecec370

Co-authored-by: Wenjun Ruan <wenjun@apache.org>

pegasas dismissed Radeity’s stale review via ecec370 May 30, 2024 16:01

Merge branch 'dev' into issues/16028

6ebc316

Merge branch 'dev' into issues/16028

155c1cf

rickchengx approved these changes Jun 12, 2024

View reviewed changes

rickchengx added 3 commits June 15, 2024 11:27

Merge branch 'dev' into issues/16028

22f8ec8

Merge branch 'dev' into issues/16028

7b5f248

Merge branch 'dev' into issues/16028

f36175f

rickchengx requested a review from SbloodyS as a code owner June 21, 2024 07:59

github-actions bot added kubernetes document labels Jun 21, 2024

SbloodyS approved these changes Jun 21, 2024

View reviewed changes

SbloodyS merged commit 4a76a07 into apache:dev Jun 21, 2024
63 of 64 checks passed

SbloodyS added 3.2.2 and removed 3.3.0 labels Jun 26, 2024

SbloodyS modified the milestones: 3.3.0, 3.2.2 Jun 26, 2024

SbloodyS mentioned this pull request Jun 26, 2024

[Chore] [Cherry-Pick] cherry pick some pr to 3.2.2 #16215

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Improvement][Helm] Make configmap of api/master/worker/alert configuration #16058

[Improvement][Helm] Make configmap of api/master/worker/alert configuration #16058

pegasas commented May 23, 2024

Gallardot left a comment

Radeity commented May 23, 2024

pegasas commented May 27, 2024

codecov-commenter commented May 27, 2024 •

edited

Loading

Radeity commented May 27, 2024

Gallardot May 27, 2024

pegasas May 27, 2024

Gallardot May 27, 2024

pegasas May 27, 2024

Gallardot May 27, 2024

pegasas May 27, 2024

Radeity commented May 28, 2024

pegasas commented Jun 11, 2024

rickchengx left a comment

SbloodyS left a comment

sonarcloud bot commented Jun 21, 2024

sonarcloud bot commented Jun 21, 2024

[Improvement][Helm] Make configmap of api/master/worker/alert configuration #16058

[Improvement][Helm] Make configmap of api/master/worker/alert configuration #16058

Conversation

pegasas commented May 23, 2024

Purpose of the pull request

Brief change log

Verify this pull request

Gallardot left a comment

Choose a reason for hiding this comment

Radeity commented May 23, 2024

pegasas commented May 27, 2024

codecov-commenter commented May 27, 2024 • edited Loading

Codecov Report

Radeity commented May 27, 2024

Gallardot May 27, 2024

Choose a reason for hiding this comment

pegasas May 27, 2024

Choose a reason for hiding this comment

Gallardot May 27, 2024

Choose a reason for hiding this comment

pegasas May 27, 2024

Choose a reason for hiding this comment

Gallardot May 27, 2024

Choose a reason for hiding this comment

pegasas May 27, 2024

Choose a reason for hiding this comment

Radeity commented May 28, 2024

pegasas commented Jun 11, 2024

rickchengx left a comment

Choose a reason for hiding this comment

SbloodyS left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 21, 2024

Quality Gate passed

sonarcloud bot commented Jun 21, 2024

Quality Gate passed

codecov-commenter commented May 27, 2024 •

edited

Loading