Skip to content

Added function to use a config directory other than .onos#220

Open
AntonJoha wants to merge 4 commits intoonosproject:masterfrom
AntonJoha:master
Open

Added function to use a config directory other than .onos#220
AntonJoha wants to merge 4 commits intoonosproject:masterfrom
AntonJoha:master

Conversation

@AntonJoha
Copy link
Copy Markdown

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

import (
	logging "github.com/onosproject/onos-lib-go/pkg/logging"
)
func init() {
    logging.CustomInit(".myconfig")
}
func GetLogger(names ...string) logging.Logger {
    return logging.GetLogger(names)
}

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 the CustomInit 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.

@onf-bot
Copy link
Copy Markdown
Contributor

onf-bot commented Apr 4, 2023

Can one of the admins verify this patch?

@SeanCondon
Copy link
Copy Markdown

retest this please

@SeanCondon
Copy link
Copy Markdown

this is giving a build error

09:12:32 pkg/logging/config.go:278: File is not `gofmt`-ed with `-s` (gofmt)
09:12:32 
09:12:32 pkg/logging/logger.go:19:1: exported: exported function CustomInit should have comment or be unexported (revive)
09:12:32 func CustomInit(dir string) {
09:12:32 ^

@AntonJoha
Copy link
Copy Markdown
Author

this is giving a build error

09:12:32 pkg/logging/config.go:278: File is not `gofmt`-ed with `-s` (gofmt)
09:12:32 
09:12:32 pkg/logging/logger.go:19:1: exported: exported function CustomInit should have comment or be unexported (revive)
09:12:32 func CustomInit(dir string) {
09:12:32 ^

If you're interested then I could set aside time to fix it in a day or two

@SeanCondon
Copy link
Copy Markdown

If you're interested then I could set aside time to fix it in a day or two
OK - it's just that I don't think anyone will review it if it's not passing tests

@gab-arrobo
Copy link
Copy Markdown

ok to test

@AntonJoha
Copy link
Copy Markdown
Author

ok to test

Sorry, forgot about this PR, pushed a commit to pass the comment requirement check.

@gab-arrobo
Copy link
Copy Markdown

@AntonJoha you need to apply gofmt to the files you are updating. That is:
gofmt -w pkg/logging/config.go and gofmt -w pkg/logging/logger.go

Co-authored-by: gab-arrobo <gabriel.arrobo@intel.com>
@gab-arrobo
Copy link
Copy Markdown

@SeanCondon, what are your thoughts about this PRs?

viper.SetConfigName("logging")

// Set the path to look for the configurations file
viper.AddConfigPath("./" + configDir + "/config")
Copy link
Copy Markdown

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

Copy link
Copy Markdown
Author

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.

Copy link
Copy Markdown

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants