Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

log: fix log file path #345

Merged
merged 2 commits into from
Jul 8, 2020
Merged

log: fix log file path #345

merged 2 commits into from
Jul 8, 2020

Conversation

glorv
Copy link
Contributor

@glorv glorv commented Jul 7, 2020

What problem does this PR solve?

fix lightning always set log file path to the default path

What is changed and how it works?

Only set log path to default path if neither command line params nor config file set it

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

Related changes

@glorv glorv requested review from kennytm and lichunzhu July 7, 2020 07:01
Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -197,6 +197,9 @@ func LoadGlobalConfig(args []string, extraFlags func(*flag.FlagSet)) (*GlobalCon
if *logFilePath != "" {
cfg.App.Config.File = *logFilePath
}
if cfg.App.Config.File == "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if cfg.App.Config.File == "" {
else {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This else is not ok, because toml config may set the log config path

Copy link
Collaborator

@kennytm kennytm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should still be a way to explicitly log to terminal

@glorv
Copy link
Contributor Author

glorv commented Jul 8, 2020

there should still be a way to explicitly log to terminal

Then I think we should leave the default to stdout

@kennytm
Copy link
Collaborator

kennytm commented Jul 8, 2020

We could make --log-file "-" to mean stdout, or copy BR's $BR_LOG_TO_TERM env var

@glorv
Copy link
Contributor Author

glorv commented Jul 8, 2020

We could make --log-file "-" to mean stdout, or copy BR's $BR_LOG_TO_TERM env var

So shall we need to update docs for this special log config. Seems br special env $BR_LOG_TO_TERM is not mensioned in docs

@kennytm kennytm added Should Update Ansible The config in TiDB-Ansible should be updated Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated labels Jul 8, 2020
@kennytm kennytm requested a review from lichunzhu July 8, 2020 08:11
Copy link
Collaborator

@kennytm kennytm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, @lichunzhu PTAL again.

@kennytm kennytm added the status/LGT1 One reviewer already commented LGTM (LGTM1) label Jul 8, 2020
Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lichunzhu lichunzhu added status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) and removed status/LGT1 One reviewer already commented LGTM (LGTM1) labels Jul 8, 2020
@kennytm kennytm merged commit 25256f7 into master Jul 8, 2020
@kennytm kennytm deleted the fix-log branch July 8, 2020 09:43
@overvenus overvenus added this to the v4.0.3 milestone Jul 15, 2020
@glorv glorv mentioned this pull request Aug 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Should Update Ansible The config in TiDB-Ansible should be updated Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants