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

Is there a way to share variables inside cobra instance? #1289

Closed
kotyara85 opened this issue Dec 3, 2020 · 7 comments
Closed

Is there a way to share variables inside cobra instance? #1289

kotyara85 opened this issue Dec 3, 2020 · 7 comments

Comments

@kotyara85
Copy link

I need to share some variables across all CMDs I use.
The problem is that one of those is logger object which supposed to change to debug mode once --debug flag is set, but that flag wont be set until PreRun()
The one option would be to use context, but of course we cant set up after we execute, so debug flag will be ignored in this case.
Help is highly appreciated
Thanks

@apapsch
Copy link

apapsch commented Dec 4, 2020

Persistent flags are available to all commands.

To store the logger, there's always global variables, but don't do that. I had my logger as global variable as well, but for testing it needed to be replaced with a specially prepared logger. Thus I opened the "writing to global variables" can of worms.

Usually func main is very empty, just invoking cobra functions. So, one might say, your commands Execute function is the real entry point. The first thing you might do is create a logger:

Execute: func(cmd *cobra.Command, args []string) {
    logger := CreateMyLogger(debug)
}

debug in the code above needs to be replaced with your debug variable, which could be initialized with cmd.Flags().GetBool(...).

I use zap logger, where child loggers can be easily created, including raising log level (logger.WithOptions(zap.IncreaseLevel(...)). This allows for a root logger that is always available, even in the bare main. See https://goplay.space/#UNV8bRM25Oh for an example (it does not run).

While this approach works for me, I would also be very interested how others solve the issue.

@kotyara85
Copy link
Author

kotyara85 commented Dec 4, 2020

I just went with this code, similar what viper does

package logger

import (
	log "github.com/sirupsen/logrus"
	"github.com/spf13/viper"
)

var logger *log.Logger

func init() {
	logger = log.New()
}

func Info(msg ...string) {
	logger.Info(msg)
}

func Debug(msg ...string) {
	debug()
	logger.Debug(msg)
}

func Error(msg ...string) {
	logger.Error(msg)
}

func Fatal(err ...error) {
	logger.Fatal(err)
}

func debug() {
	if viper.GetBool("debug") {
		logger.Level = log.DebugLevel
	}
}
```

After you just import it in any other package and run `logger.Info("message")`

@apapsch
Copy link

apapsch commented Dec 7, 2020

If it works for you and doesn't interfere with your tests (due to logger being a global variable), go with it. Though one observation: your adapter API does not allow for structured logging. I found structured logging super useful with the zap logger and logrus supports it too (they pioneered it in Go land). It's useful not only for log querying in some analysis tool, but also for integration tests.

First I create a special logger which records the logs:

coreLogger, recordedLogs := observer.New(zapcore.InfoLevel)
rootLogger := zap.New(coreLogger)

Root logger gets set in a context which is passed to ExecuteContext of a Cobra.Command and the commands Execute receives it. After the command does its thing, recorded logs can be filtered:

recordedLogs = recordedLogs.FilterField(zap.String("foo", "bar"))
if recordedLogs.Len() == 0 {
    t.Fatalf("no good")
}

@github-actions
Copy link

github-actions bot commented Feb 6, 2021

This issue is being marked as stale due to a long period of inactivity

@CDimonaco
Copy link

The real problem with the context approach is that there is no API to augment the context later, we can retrieve the context in commands but not reassign. Cobra offers a lot of capabilities but the lack of a dependency injection mechanism is really a pain point. Wrappers and empty interface cannot substitute DI.

@MrFastDie
Copy link

The real problem with the context approach is that there is no API to augment the context later, we can retrieve the context in commands but not reassign. Cobra offers a lot of capabilities but the lack of a dependency injection mechanism is really a pain point. Wrappers and empty interface cannot substitute DI.

Do you know if theres anything planned to reassign the context? Right now I have to use global variables and I don't really like that.

@johnSchnake
Copy link
Collaborator

There are two PRs currently under consideration for setting the context and facilitating it to be chained from prerun->run->postrun See #1551 #1118

It seems like there isn't anything new in this issue then and you can track those PRs or their underlying issues to stay abreast of that solution.

Feel free to reopen this if you feel this ticket requires more discussion.

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

No branches or pull requests

5 participants