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

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

AntonJoha
Copy link

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
Contributor

onf-bot commented Apr 4, 2023

Can one of the admins verify this patch?

@SeanCondon
Copy link
Contributor

retest this please

@SeanCondon
Copy link
Contributor

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
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
Contributor

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

ok to test

@AntonJoha
Copy link
Author

ok to test

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

@gab-arrobo
Copy link

@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

pkg/logging/logger.go Outdated Show resolved Hide resolved
@gab-arrobo
Copy link

@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")
Copy link
Contributor

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
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
Contributor

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