-
Notifications
You must be signed in to change notification settings - Fork 35
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
Added function to use a config directory other than .onos #220
base: master
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? |
retest this please |
this is giving a build error
|
If you're interested then I could set aside time to fix it in a day or two |
|
ok to test |
Sorry, forgot about this PR, pushed a commit to pass the comment requirement check. |
@AntonJoha you need to apply |
Co-authored-by: gab-arrobo <[email protected]>
@SeanCondon, what are your thoughts about this PRs? |
@@ -286,8 +285,8 @@ func load(config *Config) error { | |||
viper.SetConfigName("logging") | |||
|
|||
// Set the path to look for the configurations file | |||
viper.AddConfigPath("./" + configDir + "/config") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be simpler to have changed line 12 to load configdir
from an environment variable. If not present it can be .onos
by default, so as not to break existing functionality.
Reading of this env var could be done in the init()
function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What existing behaviour does it break? I guess using a env-var is up to personal style so don't mind that part.
I didn't mention it earlier but the init function is (as far as I can see) a bit of an issue as well. As it contains code in its path that can panic. It is to my knowledge not possible to handle this panic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point on the panic inside init().
As far as existing functionality is concerned - I mean existing uses expect a default value .onos
if not ENV VAR is present
Hi.
I am not sure if this project is still under active use or if a pull request is appreciated.
But I found that the logging library here gave us with a simple method of logging to a Kafka broker.
The only issue I had is that we in our project would like to save all configurations in the same directory, meaning that the standard ".onos" folder is a hard no.
This pull request aims to fix that by introducing the function
CustomInit
function.It gives the user of the library a chance of initializing with a custom directory name.
This is done in our case with a wrapper library which calls
CustomInt
in the init function.The wrapper library can then be added to use a custom directory.
Code below is how the basic wrapper could look
This means that the init function in the file pkg/logging/logger.go had to be removed, else it would init on load before the user can call
CustomInit
.A nil check for the variable root has been added in the function
GetLogger
to handle the cases where a user doesn't want to use theCustomInit
function.This means that users which want the library to work the same as before can simply call the
GetLogger
and get the same expected behaviour.This adds an extra check every time the function is called which is extra overhead. But I'm not sure if that amount is of any care to the project?
Again, I don't know if anyone still manages this repository and if pull requests are appreciated.
I'd be happy to change the code or further explain my reasoning if anyone is interest!
Thank you for taking the time to read this.