[Feature][Helm] make api/master/worker/alert application.yaml configurable #15922
Conversation
| @@ -794,6 +794,97 @@ alert: | |||
| labels: {} | |||
| # -- serviceMonitor.annotations ServiceMonitor annotations | |||
| annotations: {} | |||
| configmap: | |||
There was a problem hiding this comment.
I don't think we need to put everything into values. In the future, probably we will have separated configmaps for api server, master server and worker server. Why not put them directly in configmap-dolphinscheduler-alert.yam?
There was a problem hiding this comment.
I don't think we need to put everything into values. In the future, probably we will have separated configmaps for api server, master server and worker server. Why not put them directly in
configmap-dolphinscheduler-alert.yam?
it seems like 2 points?
1.not put everything into values, i agree. maybe we can choose some important items here.
2.because we can not just put application.yaml into cm but other config items. i think it may be more extendable if configured in values.yaml
There was a problem hiding this comment.
We could put application.yaml into configmap, and make some important items of application.yaml as variables and access them through values.yaml. For example api server, alert server and master server all need access to db and there are db related configs in the application.yaml for all of them. So you may put db related configs in values.yaml, and share those configs across different application.yaml for alert server, api server and master server. The same for registry related configs.
There was a problem hiding this comment.
We could put application.yaml into configmap, and make some important items of application.yaml as variables and access them through
values.yaml. For example api server, alert server and master server all need access to db and there are db related configs in theapplication.yamlfor all of them. So you may put db related configs in values.yaml, and share those configs across differentapplication.yamlfor alert server, api server and master server. The same for registry related configs.
I got your point.
will apply these changes.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #15922 +/- ##
=========================================
Coverage 40.58% 40.58%
- Complexity 5215 5216 +1
=========================================
Files 1380 1380
Lines 46056 46056
Branches 4916 4916
=========================================
+ Hits 18692 18693 +1
Misses 25437 25437
+ Partials 1927 1926 -1 ☔ View full report in Codecov by Sentry. |
pegasas
changed the title
| registry: | ||
| type: zookeeper | ||
| zookeeper: | ||
| namespace: dolphinscheduler | ||
| connect-string: localhost:2181 | ||
| retry-policy: | ||
| base-sleep-time: 60ms | ||
| max-sleep: 300ms | ||
| max-retries: 5 | ||
| session-timeout: 30s | ||
| connection-timeout: 9s | ||
| block-until-connected: 600ms | ||
| digest: ~ |
There was a problem hiding this comment.
Can these move into values.yaml?
There was a problem hiding this comment.
this pr will add configmap and split db config only,
then will raise another to split.registry.
finally maybe will deprecate old db registry config.
| datasource: | ||
| profile: postgresql | ||
| config: | ||
| driver-class-name: org.postgresql.Driver | ||
| url: jdbc:postgresql://127.0.0.1:5432/dolphinscheduler | ||
| username: root | ||
| password: root | ||
| hikari: | ||
| connection-test-query: select 1 | ||
| pool-name: DolphinScheduler |
There was a problem hiding this comment.
It's incompatible with old db config, cuz ENV has higher priority.
Additionally, I recommend that enable option for different datasource should be provided here like before.
There was a problem hiding this comment.
I've already consider this situation.
now application.yaml in docker are all configured like this way and env will overwrite this config.
so i kept old db config above so that they will become env to overwrite.
it is compatible already.
There was a problem hiding this comment.
Can I understand that you just want to define new db configs firstly in this PR and you will replace old db configs in future PR?
There was a problem hiding this comment.
#15924 (comment)
Exactly,
I will abandon that in next following plan.
There was a problem hiding this comment.
Got it. If you want to do this step by step, I suggest you in this PR, just define new configMap and do not modify values.yaml. In future PR, replace old db/registry config with new db/registry all together.
From my perspective, this way is clearer :D
There was a problem hiding this comment.
sure,
this is better
i will follow this guideline here
There was a problem hiding this comment.
Got it. If you want to do this step by step, I suggest you in this PR, just define new configMap and do not modify
values.yaml. In future PR, replace old db/registry config with new db/registry all together.From my perspective, this way is clearer :D
Done.
|
Purpose of the pull request
fix #15473
part of #15924
Brief change log
make Alert application.yaml configurable
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