-
Notifications
You must be signed in to change notification settings - Fork 7
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
Moved ConfigFromEnv function to mint package #95
base: main
Are you sure you want to change the base?
Conversation
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.
hey, thanks for the suggestion. I actually had it like this before where I had that method to get the configuration from the env in the mint package. However I moved it out because that was specific to the mint binary and how I'm loading the env vars from the .env file. It also defaults to $HOME/.gonuts/mint for the db path which I wouldn't want to assume inside the mint package.
So I thought anyone using the package would provide the config struct. Outside of the package you can get the config however you want (env, json, yaml, etc.)
dbMigrations := os.Getenv("DB_MIGRATIONS") | ||
if len(dbMigrations) == 0 { | ||
dbMigrations = "../../mint/storage/sqlite/migrations" | ||
} |
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 should probably fix this. This is very bad how I'm hardcoding the migrations. Think I need to embed them so that is not even an option
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.
Ok thats true. I have added a DB_MIGRATIONS
variable to avoid this. We could fix the default values by using functional option parameters in the configuration function.
I wanted to reuse the ConfigFromEnv function, because i also want to use the same ENV variables as gonuts does.
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 did this #96 for the migrations which embeds the files. So you should be able to remove it since it is not in the config anymore.
mintPath := os.Getenv("MINT_DB_PATH") | ||
// if MINT_DB_PATH is empty, use $HOME/.gonuts/mint | ||
if len(mintPath) == 0 { | ||
homedir, err := os.UserHomeDir() | ||
if err != nil { | ||
return nil, err | ||
} | ||
mintPath = filepath.Join(homedir, ".gonuts", "mint") | ||
} |
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.
if moving this to the mint package, I don't think it should default to this directory. Think it would be better to return an error if empty and then application-specific can provide a default directory if the error happens.
I am currently trying to initialize a gonuts mint within NWS exit nodes.
Moving this configuration function into a public package is quite helpful for integrating gonuts.
Additionally, I have updated the error statements to wrap errors correctly using the
"%w
" placeholder.