You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@devlake.apache.org by GitBox <gi...@apache.org> on 2022/06/07 09:37:24 UTC
[GitHub] [incubator-devlake] SnowMoon-Dev opened a new pull request, #2106: [fix#2055][config] optimize configuration initialization function and add endpoint debug log
SnowMoon-Dev opened a new pull request, #2106:
URL: https://github.com/apache/incubator-devlake/pull/2106
optimize configuration initialization function and add endpoint debug log
close [#2055](https://github.com/apache/incubator-devlake/issues/2055)
## Verify this pull request
Manually verified the change by testing locally.
--
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@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] matrixji commented on a diff in pull request #2106: [fix#2055][config] optimize configuration initialization function and add endpoint debug log
Posted by GitBox <gi...@apache.org>.
matrixji commented on code in PR #2106:
URL: https://github.com/apache/incubator-devlake/pull/2106#discussion_r891821617
##########
config/config_test.go:
##########
@@ -18,57 +18,42 @@ limitations under the License.
package config
import (
- "os"
- "testing"
-
- "github.com/spf13/viper"
+ "github.com/sirupsen/logrus"
+ "github.com/spf13/afero"
"github.com/stretchr/testify/assert"
+ "testing"
)
-func TestReadAndWriteToConfig(t *testing.T) {
+func TestReadConfig(t *testing.T) {
+ DbUrl := "mysql://merico:merico@mysql:3306/lake?charset=utf8mb4&parseTime=True"
v := GetConfig()
currentDbUrl := v.GetString("DB_URL")
- newDbUrl := "ThisIsATest"
- assert.Equal(t, currentDbUrl != newDbUrl, true)
- v.Set("DB_URL", newDbUrl)
- err := v.WriteConfig()
- assert.Equal(t, err == nil, true)
- nowDbUrl := v.GetString("DB_URL")
- assert.Equal(t, nowDbUrl == newDbUrl, true)
- // Reset back to current
- v.Set("DB_URL", currentDbUrl)
- err = v.WriteConfig()
- assert.Equal(t, err == nil, true)
+ logrus.Infof("current db url: %s\n", currentDbUrl)
+ assert.Equal(t, currentDbUrl == DbUrl, true)
}
-func TestGetEnvPath(t *testing.T) {
- os.Unsetenv("ENV_PATH")
- assert.Equal(t, getEnvPath(), ".env")
- os.Setenv("ENV_PATH", "/foo/bar/config.env")
- assert.Equal(t, getEnvPath(), "/foo/bar/config.env")
+func TestWriteConfig(t *testing.T) {
+ filename := ".env"
+ v := GetConfig()
+ newDbUrl := "mysql://merico:merico@mysql:3307/lake?charset=utf8mb4&parseTime=True"
+ v.Set("DB_URL", newDbUrl)
+ fs := afero.NewOsFs()
+ file, _ := fs.Create(filename)
+ defer file.Close()
+ _ = WriteConfigAs(v, filename)
+ isEmpty, _ := afero.IsEmpty(fs, filename)
+ assert.False(t, isEmpty)
+ err := fs.Remove(filename)
+ assert.Equal(t, err == nil, true)
}
-func TestWriteConfigToEnvPath(t *testing.T) {
Review Comment:
We'd better not remove the testing code, but adapt it to work for the new implementations.
--
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@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] matrixji commented on pull request #2106: [fix#2055][config] optimize configuration initialization function and add endpoint debug log
Posted by GitBox <gi...@apache.org>.
matrixji commented on PR #2106:
URL: https://github.com/apache/incubator-devlake/pull/2106#issuecomment-1149396414
> > Short question, currently we using **ENV_PATH=/app/config/.env** in helm deployment, does this PR requires we change **ENV_PATH** to **/app/config** ? If so, we could adapt it after this PR merged.
>
> It seems that there is no need to modify, I deployed with HELM yesterday and successfully started the project ...
Those changes LGTM, right now.
And if possible could you help to confirm with the below steps:
- Change some configurations and save.
- Check where whether the configurations are saved to the .env with the expected path
--
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@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] klesh commented on pull request #2106: [fix#2055][config] optimize configuration initialization function and add endpoint debug log
Posted by GitBox <gi...@apache.org>.
klesh commented on PR #2106:
URL: https://github.com/apache/incubator-devlake/pull/2106#issuecomment-1148658668
Hi， Can you provide some screen-shot of the logging output?
--
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@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] SnowMoon-Dev commented on pull request #2106: [fix#2055][config] optimize configuration initialization function and add endpoint debug log
Posted by GitBox <gi...@apache.org>.
SnowMoon-Dev commented on PR #2106:
URL: https://github.com/apache/incubator-devlake/pull/2106#issuecomment-1149392390
> Short question, currently we using **ENV_PATH=/app/config/.env** in helm deployment, does this PR requires we change **ENV_PATH** to **/app/config** ? If so, we could adapt it after this PR merged.
It seems that there is no need to modify, I deployed with HELM yesterday and successfully started the project
--
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@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] warren830 merged pull request #2106: [fix#2055][config] optimize configuration initialization function and add endpoint debug log
Posted by GitBox <gi...@apache.org>.
warren830 merged PR #2106:
URL: https://github.com/apache/incubator-devlake/pull/2106
--
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@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] warren830 commented on pull request #2106: [fix#2055][config] optimize configuration initialization function and add endpoint debug log
Posted by GitBox <gi...@apache.org>.
warren830 commented on PR #2106:
URL: https://github.com/apache/incubator-devlake/pull/2106#issuecomment-1149489205
Thank you @SnowMoon-Dev @matrixji , you are super hero for us!
--
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@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] SnowMoon-Dev commented on pull request #2106: [fix#2055][config] optimize configuration initialization function and add endpoint debug log
Posted by GitBox <gi...@apache.org>.
SnowMoon-Dev commented on PR #2106:
URL: https://github.com/apache/incubator-devlake/pull/2106#issuecomment-1148662303
> Hi， Can you provide some screen-shot of the logging output?
<img width="1025" alt="image" src="https://user-images.githubusercontent.com/42627702/172389657-4d752bba-bedf-4ef1-943f-3adf04cf2c1b.png">
<img width="1062" alt="image" src="https://user-images.githubusercontent.com/42627702/172389733-5b09adec-07ec-404d-9d99-01f2eba32d85.png">
self-test didn't find any problems
--
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@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] warren830 commented on pull request #2106: [fix#2055][config] optimize configuration initialization function and add endpoint debug log
Posted by GitBox <gi...@apache.org>.
warren830 commented on PR #2106:
URL: https://github.com/apache/incubator-devlake/pull/2106#issuecomment-1149906746
> > > Short question, currently we using **ENV_PATH=/app/config/.env** in helm deployment, does this PR requires we change **ENV_PATH** to **/app/config** ? If so, we could adapt it after this PR merged.
> >
> >
> > It seems that there is no need to modify, I deployed with HELM yesterday and successfully started the project ...
>
> Those changes LGTM, right now.
>
> And if possible could you help to confirm with the below steps:
>
> * Change some configurations and save.
> * Check where whether the configurations are saved to the .env with the expected path
I deployed through helm, and it works very well, thank you Bin @matrixji ! And thank you Shimin @SnowMoon-Dev !
--
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@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] matrixji commented on pull request #2106: [fix#2055][config] optimize configuration initialization function and add endpoint debug log
Posted by GitBox <gi...@apache.org>.
matrixji commented on PR #2106:
URL: https://github.com/apache/incubator-devlake/pull/2106#issuecomment-1149323205
Short question, currently we using **ENV_PATH=/app/config/.env** in helm deployment, does this PR requires we change **ENV_PATH** to **/app/config** ? If so, we could adapt it after this PR merged.
--
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@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org