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

fix: Change output to outputpath in config file template for logger #716

Merged
merged 4 commits into from
Aug 8, 2022

Conversation

fredcarle
Copy link
Collaborator

Relevant issue(s)

Resolves #715

Description

This PR changes the config file template to have the right text for the output path of the logger.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

manual and unit tests

Specify the platform(s) on which this was tested:

  • MacOS

@fredcarle fredcarle added bug Something isn't working area/logging Related to the logging/logger system action/no-benchmark Skips the action that runs the benchmark. area/config Related to configuration labels Aug 3, 2022
@fredcarle fredcarle added this to the DefraDB v0.3.1 milestone Aug 3, 2022
@fredcarle fredcarle requested a review from a team August 3, 2022 05:21
@fredcarle fredcarle self-assigned this Aug 3, 2022
@fredcarle fredcarle force-pushed the fredcarle/fix/I715-logger-output-path branch from cb11af4 to 19fd372 Compare August 3, 2022 06:15
Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

LGTM!

@orpheuslummis
Copy link
Contributor

With defradb's configuration we can set a parameter via multiple ways: CLI flags, env variables, configuration file. One principle is to keep the param name the same across.

Here, I suggest the param is to be called output rather than outputpath because "path" is implied.

That would mean changing

err = viper.BindPFlag("log.outputpath", rootCmd.PersistentFlags().Lookup("logoutput"))

to log.output

and keeping the output key in config file. It's the key in config file template that defines the env variable that is detected by viper.

@orpheuslummis
Copy link
Contributor

cheers for writing the test

@fredcarle fredcarle force-pushed the fredcarle/fix/I715-logger-output-path branch from 19fd372 to 4eace7c Compare August 3, 2022 16:58
@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #716 (ccc94de) into develop (38613a5) will not change coverage.
The diff coverage is 66.66%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #716   +/-   ##
========================================
  Coverage    57.48%   57.48%           
========================================
  Files          143      143           
  Lines        16390    16390           
========================================
  Hits          9422     9422           
  Misses        6085     6085           
  Partials       883      883           
Impacted Files Coverage Δ
cli/cli.go 18.75% <0.00%> (ø)
config/configfile.go 75.00% <ø> (ø)
cli/root.go 47.67% <66.66%> (ø)
config/config.go 74.60% <100.00%> (ø)

@fredcarle
Copy link
Collaborator Author

@orpheuslummis and @shahzadlone are you guys happy with the latest changes based on feedback?

@shahzadlone
Copy link
Member

@orpheuslummis and @shahzadlone are you guys happy with the latest changes based on feedback?

LGTM

Copy link
Contributor

@orpheuslummis orpheuslummis left a comment

Choose a reason for hiding this comment

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

LGTM

@fredcarle fredcarle force-pushed the fredcarle/fix/I715-logger-output-path branch from 04b4d20 to ccc94de Compare August 8, 2022 18:58
@fredcarle fredcarle merged commit e00ca5a into develop Aug 8, 2022
@fredcarle fredcarle deleted the fredcarle/fix/I715-logger-output-path branch August 8, 2022 19:20
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
…ourcenetwork#716)

Relevant issue(s)
Resolves sourcenetwork#715

Description
This PR changes the config file template to have the right text for the output path of the logger.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/config Related to configuration area/logging Related to the logging/logger system bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

logger output path in config is not marshalled to struct field
3 participants