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/08/19 14:36:35 UTC
[GitHub] [incubator-devlake] keon94 opened a new pull request, #2781: [feat-2742]: Add default logging directory
keon94 opened a new pull request, #2781:
URL: https://github.com/apache/incubator-devlake/pull/2781
### ⚠️ Pre Checklist
> Please complete _ALL_ items in this checklist, and remove before submitting
- [ ] I have read through the [Contributing](https://devlake.apache.org/community/) Documentation & [PR Template](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
- [ ] This PR is using a `label` (bug, feature etc.)
- [ ] My code is has necessary documentation (if appropriate)
- [ ] I have added any relevant tests
- [ ] This section (**⚠️ Pre Checklist**) will be removed when submitting PR
# Summary
<!--
Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes.
Please fill out as many sections below as possible.
-->
### Does this close any open issues?
#2742
### Screenshots
Include any relevant screenshots here.
### Other Information
Any other information that is important to this 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@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] keon94 commented on pull request #2781: [feat-2742]: Add default logging directory
Posted by GitBox <gi...@apache.org>.
keon94 commented on PR #2781:
URL: https://github.com/apache/incubator-devlake/pull/2781#issuecomment-1222860762
> Hi, @keon94 I suggest that we go with "no LOGGING_DIR, no logging files", using a **relative directory** would cause multiple dirs to be created
Hey Klesh, I've added LOGGING_DIR to docker-compose instead with volume mapping like we discussed earlier today.
--
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 #2781: [feat-2742]: Handle logging config
Posted by GitBox <gi...@apache.org>.
klesh commented on PR #2781:
URL: https://github.com/apache/incubator-devlake/pull/2781#issuecomment-1223419905
Good job!! I like it overall. Please fix the minor issues, and we can merge and test.
--
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 merged pull request #2781: [feat-2742]: Handle logging config
Posted by GitBox <gi...@apache.org>.
klesh merged PR #2781:
URL: https://github.com/apache/incubator-devlake/pull/2781
--
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 a diff in pull request #2781: [feat-2742]: Handle logging config
Posted by GitBox <gi...@apache.org>.
klesh commented on code in PR #2781:
URL: https://github.com/apache/incubator-devlake/pull/2781#discussion_r952064361
##########
services/pipeline.go:
##########
@@ -389,13 +389,13 @@ func CancelPipeline(pipelineId uint64) error {
// getPipelineLogsPath gets the logs directory of this pipeline
func getPipelineLogsPath(pipeline *models.Pipeline) (string, error) {
pipelineLog := getPipelineLogger(pipeline)
- path := filepath.Dir(pipelineLog.GetConfig().Path)
+ path := pipelineLog.GetConfig().Path
_, err := os.Stat(path)
if err == nil {
- return path, nil
+ return filepath.Dir(path), nil
}
if os.IsNotExist(err) {
- return "", fmt.Errorf("logs for pipeline #%d not found: %v", pipeline.ID, err)
+ return "", fmt.Errorf("logs for pipeline #%d not found: %w", pipeline.ID, err)
Review Comment:
Can we add more hint to the error message? like "you have to specify the LOGGING_DIR first"
##########
.env.example:
##########
@@ -27,7 +27,6 @@ TEMPORAL_URL=
TEMPORAL_TASK_QUEUE=
# Debug Info Warn Error
LOGGING_LEVEL=
-LOGGING_DIR=./logs
Review Comment:
We can keep this key, but let it be empty.
--
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] keon94 commented on pull request #2781: [feat-2742]: Add default logging directory
Posted by GitBox <gi...@apache.org>.
keon94 commented on PR #2781:
URL: https://github.com/apache/incubator-devlake/pull/2781#issuecomment-1220756059
@warren830 @hezyin could any of you guys look at and approve this?
--
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] keon94 commented on a diff in pull request #2781: [feat-2742]: Handle logging config
Posted by GitBox <gi...@apache.org>.
keon94 commented on code in PR #2781:
URL: https://github.com/apache/incubator-devlake/pull/2781#discussion_r952722497
##########
.env.example:
##########
@@ -27,7 +27,6 @@ TEMPORAL_URL=
TEMPORAL_TASK_QUEUE=
# Debug Info Warn Error
LOGGING_LEVEL=
-LOGGING_DIR=./logs
Review Comment:
done
##########
services/pipeline.go:
##########
@@ -389,13 +389,13 @@ func CancelPipeline(pipelineId uint64) error {
// getPipelineLogsPath gets the logs directory of this pipeline
func getPipelineLogsPath(pipeline *models.Pipeline) (string, error) {
pipelineLog := getPipelineLogger(pipeline)
- path := filepath.Dir(pipelineLog.GetConfig().Path)
+ path := pipelineLog.GetConfig().Path
_, err := os.Stat(path)
if err == nil {
- return path, nil
+ return filepath.Dir(path), nil
}
if os.IsNotExist(err) {
- return "", fmt.Errorf("logs for pipeline #%d not found: %v", pipeline.ID, err)
+ return "", fmt.Errorf("logs for pipeline #%d not found: %w", pipeline.ID, err)
Review Comment:
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@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] klesh commented on pull request #2781: [feat-2742]: Add default logging directory
Posted by GitBox <gi...@apache.org>.
klesh commented on PR #2781:
URL: https://github.com/apache/incubator-devlake/pull/2781#issuecomment-1221721081
Hi, @keon94 I suggest that we go with "no LOGGING_DIR, no logging files", using a **relative directory** would cause multiple dirs to be created
--
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] keon94 commented on pull request #2781: [feat-2742]: Add default logging directory
Posted by GitBox <gi...@apache.org>.
keon94 commented on PR #2781:
URL: https://github.com/apache/incubator-devlake/pull/2781#issuecomment-1223009348
Returning 404 if log file is not present now. FYI @e2corporation @klesh
--
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