Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #967 +/- ##
==========================================
- Coverage 74.46% 74.35% -0.12%
==========================================
Files 62 66 +4
Lines 3008 3193 +185
==========================================
+ Hits 2240 2374 +134
- Misses 605 643 +38
- Partials 163 176 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This will make testing easier, as it's more similar to the actual prod deployment. We don't have to rewrite anything in vite, as the host and origin is the same.
|
@eternal-flame-AD If you have time, could you have a look at this? (It's not urgent) |
eternal-flame-AD
left a comment
eternal-flame-AD
left a comment
There was a problem hiding this comment.
To be frank I'm not super sold on this .. could you clarify what is the problem we are solving by "standalone and understandable"?
Biggest concerns:
- It looks like the library has a .Marshal function so I would say we should at least try to make an automated config migration utility to decrease the friction to users.
- It looks like you basically unrolled the entire config parsing in config.go. Could we have achieved all the benefits you listed by adding special cases to help configor (AND achieved backwards compatibility with YAML syntax) anyways?
- The entire config discovery is done relative to the executable in a "python venv" style with only option to add one file to the front of the config list, which IMO strongly bias towards one kind of deployment, and moving the environment means the gotify executable has to move with the config file as well.
- The switch to dotenv format from YAML would encourage users to run "lean" configs with only specific items overriden. This can lead to complex problems when they are running multiple configs on the same system.
| } else { | ||
| logs = append(logs, FutureLog{ | ||
| Level: zerolog.WarnLevel, | ||
| Msg: fmt.Sprintf("cannot read file %s because %s", file, fileErr), |
There was a problem hiding this comment.
I'm not super sure we should downgrade this to a warning instead of panicking.
For one this is not the current behavior, for second this can lead to weird behaviors that are confusing. I would say at least emit INFO on which files you did find to help with debugging, and I strongly prefer any errors (except not found errors) be fatal
| "path/filepath" | ||
|
|
||
| "github.com/gotify/server/v2/mode" | ||
| "github.com/joho/godotenv" |
There was a problem hiding this comment.
I've initially wanted to use a library (https://github.com/caarlos0/env) for the env parsing, but it didn't support _FILE natively. and manually implementing it doesn't seems to complex.
Why did you use this library instead? It looks like you reimplemented the _FILE logic by hand anyways.
|
|
||
| func getFiles(relativeTo string) []string { | ||
| var result []string | ||
| if configFile := os.Getenv("GOTIFY_CONFIG_FILE"); configFile != "" { |
There was a problem hiding this comment.
I would suggest make GOTIFY_CONFIG_FILE exclusive: if that environment is present then ignore all other config files. I can't think of one but if you think there is a use case for the current behavior we should name it GOTIFY_CONFIG_INCLUDE or something.
My rationale is simply my understanding being .env files look pretty than full YAML file is you can have a basic instance running by simply overwriting very few items. This goes against that.
Let's just say a packager packaged gotify and installed it on /usr/bin/gotify . If I understand the logic correctly this would:
- first go read /usr/bin/gotify-server.env... (which hopefully will not exist)
- then it goes to $HOME/.config/gotify/gotify-server.env , there is no configurability on which environment you want and there is only one file accepted for the entire user.
- then it goes to /etc/ for a system wise config, that's okay but don't help too much
Which basically means there is no way to run two Gotify instances with the same user directory on a shared installation unless you exhaustively enumerate every config option in GOTIFY_CONFIG_FILE to prevent accidental pickup
2 participants
See the gotify-server.env.example for how it works. The new https://gotify.net/docs/config will just reference gotify-server.env.example, so it should be standalone and understandable.
I've initially wanted to use a library (https://github.com/caarlos0/env) for the env parsing, but it didn't support _FILE natively. and manually implementing it doesn't seems to complex.
Fixes #366
Fixes #392