Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/logcli: support load config from file #218

Merged
merged 1 commit into from
Feb 22, 2019

Conversation

daixiang0
Copy link
Contributor

Support load config from file and move default config into here.

Signed-off-by: Xiang Dai 764524258@qq.com

@daixiang0
Copy link
Contributor Author

#159

cmd/logcli/config.go Outdated Show resolved Hide resolved
cmd/logcli/config.go Outdated Show resolved Hide resolved
cmd/logcli/main.go Outdated Show resolved Hide resolved
@tomwilkie
Copy link
Contributor

Thanks for the PR @daixiang0! Looks like it need some more work, ping me when you've had a chance to address the feedback.

@daixiang0 daixiang0 force-pushed the support-config branch 5 times, most recently from 067cbed to 2891e28 Compare January 21, 2019 02:03
@davkal davkal self-requested a review February 15, 2019 14:19
Copy link
Contributor

@davkal davkal left a comment

Choose a reason for hiding this comment

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

  • getConfig should return with the results from LoadConfig
  • move getConfig call into main
  • handle errors and exit

cmd/logcli/main.go Outdated Show resolved Hide resolved
@daixiang0 daixiang0 force-pushed the support-config branch 3 times, most recently from 9b0723d to 0ade7a3 Compare February 20, 2019 08:02
@daixiang0
Copy link
Contributor Author

  • getConfig should return with the results from LoadConfig
  • move getConfig call into main
  • handle errors and exit

@davkal All done, please review again.

@davkal
Copy link
Contributor

davkal commented Feb 20, 2019

Isn't the error logged twice now?

Also, @gouthamve is this good code organization in the var() section?

@daixiang0
Copy link
Contributor Author

daixiang0 commented Feb 21, 2019

Isn't the error logged twice now?

Do you mean no need log in config.go? I have removed that.

Also, @gouthamve is this good code organization in the var() section?

This organization is what gofmt perfer.

docs/logcli.md Outdated Show resolved Hide resolved
docs/logcli.md Outdated Show resolved Hide resolved
docs/logcli.md Outdated Show resolved Hide resolved
@davkal
Copy link
Contributor

davkal commented Feb 21, 2019

Thank you, we're almost there. Only the docs need cleaning up, then we can merge this.

@daixiang0
Copy link
Contributor Author

Thank you, we're almost there. Only the docs need cleaning up, then we can merge this.

Done :)

Signed-off-by: Xiang Dai <764524258@qq.com>
@davkal davkal merged commit e6bf548 into grafana:master Feb 22, 2019
@davkal
Copy link
Contributor

davkal commented Feb 22, 2019

Thank you for your contribution @daixiang0 🎉

@daixiang0 daixiang0 deleted the support-config branch February 23, 2019 02:34
periklis pushed a commit to periklis/loki that referenced this pull request Dec 11, 2023
Update from upstream repository incl. go1.20 compatibily patches
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants