raystack/salt

Salt is a collection of libraries and tools used in the Raystack ecosystem to improve the experience of developing projects with Go.

salt's Issues

Add getter to get *logrus.Logger object of `salt/log`

Is your feature request related to a problem? Please describe.
grpc-middleware has support of logrus logger. However, they only accept argument with type *logrus.Logger. In salt, *logrus.Logger is private, we need to have a getter function to get the object.

Describe the solution you'd like
In this file, we could add a new function

func (l *Logrus) LogrusLogger() *logrus.Logger {
	return l.log
}

add support for grpc loggerV2

Is your feature request related to a problem? Please describe.
The current salt logger interface doesn't support grpc loggerV2 interface. Due to which in shield, we have multiple loggers at different layers operating instead of one standard logger across.

Describe the solution you'd like
Make salt logger compatible with grpc logger by refactoring the existing salt logger interface. A few of the methods have to be renamed in salt logger to be able to inject into grpc context so that we could use this logger with custom logging rules.

Add version package

Add a version package in salt which provides information if a go build is the latest version or not. This package should handle the need of just one binary(CLI) for example meteor as well as CLI and server, as an example Optimus.

Fix zap contextual logging naming

Is your feature request related to a problem? Please describe.
The contextual logging with zap in salt is inside log package. Getting zap log from context would be log.FromContext(ctx) and return Zap. Since log has multiple possible logger (logrus & zap), this would be confusing.

Describe the solution you'd like
Add more specific identifier for Zap logger specific function to make it clearer.
log.FromContext > log.ZapFromContext
log.WithFields > log.ZapContextWithFields

[salt/printer] Spinner does not allow customising writer

Is your feature request related to a problem? Please describe.

The printer.Spin() function assumes writer is STDOUT and does not allow overriding it. Showing spinner in STDOUT can cause issues when the program also writes other useful data (may be structured) to STDOUT.

# This type of usage will be common but with spinner on STDOUT, it may end up writing some
# residual characters used for the spinner rendering into the file also.
$ myprogram-using-spinner dosomework > foo.json

Describe the solution you'd like

Usually, it's better to use STDERR in for logs, progress updates, etc. or at-least if the writer itself is configurable, the client program can override the writer to implement any behaviour (no-op, write-to-stderr, write-to-stdout, etc.)

Note: The printer package seem to be assuming STDOUT for almost everything. Also, formatting functions encode into byte slice and then print. Instead, all these can be refactored to accept writer and stream-encode (e.g., json.NewEncoder(writer).Encode(v) )

not getting ConfigFileNotFoundError when config file is not present

When running the app without config file, I getting unable to read config file error instead of ConfigFileNotFoundError struct.
So i am not able to catch the error in my application side.

if err := loader.Load(&cfg); err != nil {
		if errors.As(err, &config.ConfigFileNotFoundError{}) {
			fmt.Println(err)
		} else {
                      panic(err)
		}
	}
	

The error doesn't go to if condition

Root cause of this issue is https://github.com/odpf/salt/blob/1aad792723ce5e7e0b2d55f676ba0d384b9ea09d/config/config.go#L117

due to !ok the error is going in else case.

Add dynamic command grouping support in help

Right now cmdx help provides hard coded grouping with core commands and additional commands. This can be mad flexible to dynamically group commands based on the group name provided in cobra annotations.

(config): default path set to `./` not be suitable for all usecases

In the config package the default path to load configuration from is set to ./ at https://github.com/odpf/salt/blob/main/config/config.go#L135

Which means that by default if the config is available in the ./ directory it would always load from that first.
This is not be suitable for all use cases as people might want different search order, and can cause confusion when trying to set more config paths.

While currently there is a way to override this its too cumbersome, that is to inject a custom viper instance and set the other defaults on it manually.

Removing this default and letting users use config.WithPathOption for the same will make it more clear to the users.

move audit logging pkg from guardian to salt

raystack/guardian#155
audit logger pkg was initially created in guardian. Need to move it to salt so it is reusable by everyone else.

feat: support zerolog logger for salt/log

• Add support for zerolog logger
• Support more loggers implementation #1

[bug] cmdx: config read, write and load behave differently

config read, write use yaml format while load uses salt/config which internally uses viper and mapstructure for processing configs.

This creates an issue that configs written via write cannot always be properly loaded by load function.
And read doesn't necessarily display all set configs.

[feat] Custom Error Response with two different Error() functions

Is your feature request related to a problem? Please describe.
With the current exceptions message, it's hard to get understand which part of the service is failing like data-layer, logic-layer, calling another service etc.

Describe the solution you'd like
Clear and concise details of error so it could help to understand what is the problem.
https://github.com/google/go-github/blob/master/github/github.go#L770

Describe alternatives you've considered
Create a custom Error type to add details about caller, component, request body etc. This custom type will have two different implementations for Error() methods:

• One implementation of the Error function will print the error message we want to display to end-users either as Api response or CLI response.
• Other implementations of the Error function will print enhanced error details while we logged using logger.

Add sql database common package

Is your feature request related to a problem? Please describe.
A lot of ODPF services are commonly using SQL database with Postgres driver. Most of these services end up repeating the same code to init database and define migrations.

Describe the solution you'd like
Salt can provide a shared sqlx package that provides a client for SQL database and generic methods for running migrations.

unified logging pattern & facilities

Currently salt/log package tries to abstract Logrus and Zap into a common formatted-logger interface (i.e., Infof(msg string, args ...any), etc. ). While logrus is designed as a formatted-logger, uber/zap is specifically designed for efficient structured logging and this kind of abstraction nullifies the major benefit of it.

I propose we remove this abstraction altogether¹ and assume direct usage of zap within salt and in ODPF applications that use salt. Benefits of doing this:

• All the benefits of structured logging (easy to parse logs, easy to search/filter by field values, easy to attach request context with each log, etc.)
• Not giving an abstracted formatted-logger will force us to always stick to structured logging.
• Assuming zap as the logger of choice allows us to provide certain useful utility abstractions. Few examples:
  * A request-logging middleware in mux package that automatically logs request info (method, path, client-ip, etc.) and response info (status, response time, etc.)
  * A middleware for injecting request related context (req-id, current user id, the route info, etc.) into req.Context() so that every log in all the subsequent layers automatically add this to every log entry.

Footnotes

1. We can still have some utility functions if we need to (e.g., a helper to inject log context into ctx). But attempting to abstract over logging functionality will not have justifiable benefits. ↩

Add ability to export env variables through salt

Scope/Type: config

Related discussion here.

I think that salt should have the ability to export environment variables and their respective values through a helper function; the closest we can get as of now is through the getFlattenedStructKeys function, which allows us to get keys. (which is private in nature, as it is an utility function)

It could be helpful in situations like this, where we need to get env vars. (salt could help in exporting values (more specifically, env variable key-value pair) into a map[string]string, which can then be used in populateData in meteor.)