You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by "pegasas (via GitHub)" <gi...@apache.org> on 2024/04/26 05:54:12 UTC
[PR] [Feature][Helm] make Alert application.yaml configurable [dolphinscheduler]
pegasas opened a new pull request, #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922
<!--Thanks very much for contributing to Apache DolphinScheduler, we are happy that you want to help us improve DolphinScheduler! -->
## Purpose of the pull request
fix https://github.com/apache/dolphinscheduler/issues/15473
## Brief change log
make Alert application.yaml configurable
## Verify this pull request
<!--*(Please pick either of the following options)*-->
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:
<!--*(example:)*
- *Added dolphinscheduler-dao tests for end-to-end.*
- *Added CronUtilsTest to verify the change.*
- *Manually verified the change by testing locally.* -->
(or)
If your pull request contain incompatible change, you should also add it to `docs/docs/en/guide/upgrede/incompatible.md`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make api/master/worker/alert application.yaml configurable [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#issuecomment-2114542303
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15922) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=15922&resolved=false&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=15922&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=15922&resolved=false&sinceLeakPeriod=true)
 No data about Coverage
 No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15922)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make api/master/worker/alert application.yaml configurable [dolphinscheduler]
Posted by "pegasas (via GitHub)" <gi...@apache.org>.
pegasas commented on code in PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#discussion_r1584736623
##########
deploy/kubernetes/dolphinscheduler/values.yaml:
##########
@@ -129,6 +129,17 @@ externalDatabase:
# -- The driverClassName of external database
driverClassName: "org.postgresql.Driver"
+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
Review Comment:
https://github.com/apache/dolphinscheduler/issues/15924#issue-2265227765
Exactly,
I will abandon that in next following plan.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make api/master/worker/alert application.yaml configurable [dolphinscheduler]
Posted by "pegasas (via GitHub)" <gi...@apache.org>.
pegasas commented on code in PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#discussion_r1584821879
##########
deploy/kubernetes/dolphinscheduler/values.yaml:
##########
@@ -129,6 +129,17 @@ externalDatabase:
# -- The driverClassName of external database
driverClassName: "org.postgresql.Driver"
+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
Review Comment:
sure,
this is better
i will follow this guideline here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make api/master/worker/alert application.yaml configurable [dolphinscheduler]
Posted by "Radeity (via GitHub)" <gi...@apache.org>.
Radeity commented on code in PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#discussion_r1584699781
##########
deploy/kubernetes/dolphinscheduler/values.yaml:
##########
@@ -129,6 +129,17 @@ externalDatabase:
# -- The driverClassName of external database
driverClassName: "org.postgresql.Driver"
+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
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make api/master/worker/alert application.yaml configurable [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#issuecomment-2104334172
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15922) **Quality Gate failed**
Failed conditions
 [47.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=15922&metric=new_coverage&view=list) (required ≥ 60%)
 [C Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15922) (required ≥ A)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15922)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make Alert application.yaml configurable [dolphinscheduler]
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on code in PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#discussion_r1580557409
##########
deploy/kubernetes/dolphinscheduler/values.yaml:
##########
@@ -794,6 +794,97 @@ alert:
labels: {}
# -- serviceMonitor.annotations ServiceMonitor annotations
annotations: {}
+ configmap:
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make api/master/worker/alert application.yaml configurable [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun commented on code in PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#discussion_r1584013493
##########
deploy/kubernetes/dolphinscheduler/templates/configmap-dolphinscheduler-worker.yaml:
##########
@@ -0,0 +1,108 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+{{- if and .Values.worker.enabled }}
+apiVersion: v1
+kind: ConfigMap
+metadata:
+ name: {{ include "dolphinscheduler.fullname" . }}-worker
+ labels:
+ app.kubernetes.io/name: {{ include "dolphinscheduler.fullname" . }}-worker
+ {{- include "dolphinscheduler.worker.labels" . | nindent 4 }}
+data:
+ application.yaml: |
+ spring:
+ banner:
+ charset: UTF-8
+ jackson:
+ time-zone: UTC
+ date-format: "yyyy-MM-dd HH:mm:ss"
+ autoconfigure:
+ exclude:
+ - org.springframework.boot.autoconfigure.jdbc.DataSourceAutoConfiguration
+
+ 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: ~
Review Comment:
Can these move into `values.yaml`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make Alert application.yaml configurable [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#issuecomment-2078720585
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15922) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=15922&resolved=false&inNewCodePeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=15922&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=15922&resolved=false&inNewCodePeriod=true)
 No data about Coverage
 No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15922)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make api/master/worker/alert application.yaml configurable [dolphinscheduler]
Posted by "pegasas (via GitHub)" <gi...@apache.org>.
pegasas commented on code in PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#discussion_r1584821879
##########
deploy/kubernetes/dolphinscheduler/values.yaml:
##########
@@ -129,6 +129,17 @@ externalDatabase:
# -- The driverClassName of external database
driverClassName: "org.postgresql.Driver"
+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
Review Comment:
sure, i will follow this guideline here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make api/master/worker/alert application.yaml configurable [dolphinscheduler]
Posted by "ruanwenjun (via GitHub)" <gi...@apache.org>.
ruanwenjun merged PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make Alert application.yaml configurable [dolphinscheduler]
Posted by "pegasas (via GitHub)" <gi...@apache.org>.
pegasas commented on code in PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#discussion_r1580535450
##########
deploy/kubernetes/dolphinscheduler/values.yaml:
##########
@@ -794,6 +794,97 @@ alert:
labels: {}
# -- serviceMonitor.annotations ServiceMonitor annotations
annotations: {}
+ configmap:
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make Alert application.yaml configurable [dolphinscheduler]
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on code in PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#discussion_r1580557409
##########
deploy/kubernetes/dolphinscheduler/values.yaml:
##########
@@ -794,6 +794,97 @@ alert:
labels: {}
# -- serviceMonitor.annotations ServiceMonitor annotations
annotations: {}
+ configmap:
Review 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 both 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make api/master/worker/alert application.yaml configurable [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#issuecomment-2104335946
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15922) **Quality Gate failed**
Failed conditions
 [47.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=15922&metric=new_coverage&view=list) (required ≥ 60%)
 [C Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15922) (required ≥ A)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15922)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make Alert application.yaml configurable [dolphinscheduler]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#issuecomment-2078713644
## [Codecov](https://app.codecov.io/gh/apache/dolphinscheduler/pull/15922?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 39.72%. Comparing base [(`b29965b`)](https://app.codecov.io/gh/apache/dolphinscheduler/commit/b29965bdce2b5b83c1ffe237265e6f53f01e11cf?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`5a38e49`)](https://app.codecov.io/gh/apache/dolphinscheduler/pull/15922?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> :exclamation: Current head 5a38e49 differs from pull request most recent head 3f64f3f. Consider uploading reports for the commit 3f64f3f to get more accurate results
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## dev #15922 +/- ##
============================================
+ Coverage 39.66% 39.72% +0.06%
- Complexity 5013 5033 +20
============================================
Files 1349 1349
Lines 45613 45575 -38
Branches 4891 4866 -25
============================================
+ Hits 18091 18106 +15
+ Misses 25607 25554 -53
Partials 1915 1915
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/dolphinscheduler/pull/15922?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make api/master/worker/alert application.yaml configurable [dolphinscheduler]
Posted by "pegasas (via GitHub)" <gi...@apache.org>.
pegasas commented on code in PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#discussion_r1584079121
##########
deploy/kubernetes/dolphinscheduler/templates/configmap-dolphinscheduler-worker.yaml:
##########
@@ -0,0 +1,108 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+{{- if and .Values.worker.enabled }}
+apiVersion: v1
+kind: ConfigMap
+metadata:
+ name: {{ include "dolphinscheduler.fullname" . }}-worker
+ labels:
+ app.kubernetes.io/name: {{ include "dolphinscheduler.fullname" . }}-worker
+ {{- include "dolphinscheduler.worker.labels" . | nindent 4 }}
+data:
+ application.yaml: |
+ spring:
+ banner:
+ charset: UTF-8
+ jackson:
+ time-zone: UTC
+ date-format: "yyyy-MM-dd HH:mm:ss"
+ autoconfigure:
+ exclude:
+ - org.springframework.boot.autoconfigure.jdbc.DataSourceAutoConfiguration
+
+ 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: ~
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make api/master/worker/alert application.yaml configurable [dolphinscheduler]
Posted by "pegasas (via GitHub)" <gi...@apache.org>.
pegasas commented on code in PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#discussion_r1602764755
##########
deploy/kubernetes/dolphinscheduler/values.yaml:
##########
@@ -129,6 +129,17 @@ externalDatabase:
# -- The driverClassName of external database
driverClassName: "org.postgresql.Driver"
+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
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make Alert application.yaml configurable [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#issuecomment-2078720899
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15922) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=15922&resolved=false&inNewCodePeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=15922&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=15922&resolved=false&inNewCodePeriod=true)
 No data about Coverage
 No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15922)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make Alert application.yaml configurable [dolphinscheduler]
Posted by "pegasas (via GitHub)" <gi...@apache.org>.
pegasas commented on code in PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#discussion_r1580561872
##########
deploy/kubernetes/dolphinscheduler/values.yaml:
##########
@@ -794,6 +794,97 @@ alert:
labels: {}
# -- serviceMonitor.annotations ServiceMonitor annotations
annotations: {}
+ configmap:
Review 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.
I got your point.
will apply these changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make api/master/worker/alert application.yaml configurable [dolphinscheduler]
Posted by "Radeity (via GitHub)" <gi...@apache.org>.
Radeity commented on code in PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#discussion_r1584764762
##########
deploy/kubernetes/dolphinscheduler/values.yaml:
##########
@@ -129,6 +129,17 @@ externalDatabase:
# -- The driverClassName of external database
driverClassName: "org.postgresql.Driver"
+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
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make api/master/worker/alert application.yaml configurable [dolphinscheduler]
Posted by "pegasas (via GitHub)" <gi...@apache.org>.
pegasas commented on code in PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#discussion_r1584711792
##########
deploy/kubernetes/dolphinscheduler/values.yaml:
##########
@@ -129,6 +129,17 @@ externalDatabase:
# -- The driverClassName of external database
driverClassName: "org.postgresql.Driver"
+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
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make api/master/worker/alert application.yaml configurable [dolphinscheduler]
Posted by "Radeity (via GitHub)" <gi...@apache.org>.
Radeity commented on code in PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#discussion_r1584723158
##########
deploy/kubernetes/dolphinscheduler/values.yaml:
##########
@@ -129,6 +129,17 @@ externalDatabase:
# -- The driverClassName of external database
driverClassName: "org.postgresql.Driver"
+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
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make Alert application.yaml configurable [dolphinscheduler]
Posted by "EricGao888 (via GitHub)" <gi...@apache.org>.
EricGao888 commented on code in PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#discussion_r1580525316
##########
deploy/kubernetes/dolphinscheduler/values.yaml:
##########
@@ -794,6 +794,97 @@ alert:
labels: {}
# -- serviceMonitor.annotations ServiceMonitor annotations
annotations: {}
+ configmap:
Review 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`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Feature][Helm] make api/master/worker/alert application.yaml configurable [dolphinscheduler]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #15922:
URL: https://github.com/apache/dolphinscheduler/pull/15922#issuecomment-2114545216
## [](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15922) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=15922&resolved=false&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=15922&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=15922&resolved=false&sinceLeakPeriod=true)
 No data about Coverage
 No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache-dolphinscheduler&pullRequest=15922)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org