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.
There was a problem hiding this comment.
joho/godotenv is for env file parsing. We could parse the env file ourself, but the library supports expanding variables and is maintained so doesn't seem like a bad pick.
caarlos0/env is for parsing environment variables to a go struct, so they have different use-cases and can be used together.
|
|
||
| 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
| @@ -11,7 +11,6 @@ import ( | |||
| "github.com/gin-gonic/gin" | |||
There was a problem hiding this comment.
Thanks for the review.
To be frank I'm not super sold on this .. could you clarify what is the problem we are solving by "standalone and understandable"?
I dislike having two different formats to configure gotify. E.g. the two sections in https://gotify.net/docs/config. Having a unified format with only env vars make the config section more compact. We recommend using env vars for container deployments, so I'd expect most users already use environment variables,
I also dislike the list format for environement variables of configor, there have been a couple of tickets about this, where people didn't use the [a,b] syntax for environment variables.
Edit: configor is unmaintained, with the last commit from 3 years ago.
That said, I'm perfectly fine with dropping the PR, I though right now would be a good time to do breaking changes. So you can see this as POC for a config format.
I don't think the old format is too bad, so if you're against or neutral towards this POC, I'd rather drop the PR and just implement _FILE in configor (though we'd have to fork it against then, we currently use the upstream version).
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.
Yeah, we could, but I'm not sure if it's worth the effort, I'd guess most users only have 2-5 settings configured, so just starting fresh with the new example config may be better.
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?
I dislike having two config formats, if there should be backwards compatibility then I'd rather discard this PR, as there isn't much benefit to support both formats. With a major version increase this should be the best time to make breaking changes.
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.
True, can be changed back to using the working directory. I thought we had a ticket about this, but this should be already fixed with the GOTIFY_CONFIG_FILE.
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.
I'm okay with only reading the first found config file, in this order, and then not reading other files.
Loaded from $GOTIFY_CONFIG_FILE
gotify-server.env (working directory)
$XDG_CONFIG_HOME/gotify/gotify-server.env
/etc/gotify/server.env
|
Ahh, I understand. It's your project so in terms of what should be done I completely defer to you. I think I the only thing I hope could be sorted out before merging is the friction. It's true that an average set-up require only several options to be overridden, but what's more than likely for an existing user to see is they copied the example.yml into an custom file and have modified and tuned over months or years. I don't like that suddenly you have to figure out which option you flipped and which you did not and manually transliterate it into env format. One concrete change in the current arrangement that should work better (and aligned with other software with overlay config system like openssh): Move the default values into the code itself, comment all lines out for the default config file, like this: A migration utility should be simple that just emits the default config file except uncommented overridden value when the input YAML file has a non default value for that item. We currently don't have a command line interface so how that could be integrated is uncertain. We can defer this into a separate PR but this should be doable and smooth. This has the additional benefit of making config inheritance more simple. You no longer have A -> default -> C result in C being completely masked by a fully filled default file. |
Yes, but I'd like your opinion on if you think this improves things. If not, I'd rather not merge this (:.
Okay, sounds good. Maybe we can add a cli, like this This would also allow us to add other actions to the cli, like creating an admin users.
Yeah, let's do this separately.
My reasoning for embedding the defaults values directly is, that this basically explicitly defines the default values, so when we later change the default values, they continue to have the old default values, at the time they've setup the config. But you right, with config inheritance this makes it more easier to misconfiguration. I'll comment out all the options. Do we then allow config inheritance, or only read the first found config file like described in #967 (comment)? |
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