-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Add cmd to allow generalised mapping of environment variables to Gitea Ini #7287
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7287 +/- ##
==========================================
+ Coverage 41.68% 41.69% +0.01%
==========================================
Files 528 529 +1
Lines 68507 68595 +88
==========================================
+ Hits 28557 28602 +45
- Misses 36301 36341 +40
- Partials 3649 3652 +3
Continue to review full report at Codecov.
|
@christhomas is this sufficient to map everything else? Would the colon separator be too difficult to work with? |
If I understand you correctly, you want to put colons in env var names? If so, I think that's not going to work. Try in your shell to set a env var with colons.
I must have misunderstood you? Cause I think you must be talking about something else |
Hey yeah so colons work on zsh, but yeah they're awkward: https://stackoverflow.com/questions/2821043/allowed-characters-in-linux-environment-variable-names That's why I thought I'd check. How about double underscore '__' as a separator? |
Given that we're likely to be stuck with keys only having the portable set - we do need some way of representing |
Do you have an example of a non portable character? Why would you want to represent ',' inside a key of a kv store? |
If you peruse the linked stack overflow comment you'll see that they define portable as Ini sections can have periods in their names - so we need some way of representing them. I guess providing a general escaping scheme isn't too unreasonable. I'll just have to write it that's all. |
I think the best option is to keep things simple and not overcomplicate a simple and good tool just to be compatible with a spec that gitea has no use for. It's nice to be compatible with the ini spec. But it's also true that it's way overly complex for giteas needs and starts filling the tool with ways to handle inputs it will never see. It's perfect ok, and legitimate a decision to state that for gitea. You support a subset and have it well defined. And support a certain type of input Better to be pragmatic. Right? Less code to support. Better semantics that are relevant to giteas needs. Simpler code. Gitea doesn't need this. If it does in the future. You can cross that bridge then. But I honestly think you're wasting your precious time on things that aren't important. |
Ah but we do need the period - see the configuration of subloggers or cron. There's also https://docs.gitea.io/en-us/config-cheat-sheet (I can't quite guess the anchor point right now and I'm on my phone so can't look it up.) |
OK done. We now have a completely generalised way of setting via environment variables. |
I don't think it's a good way to change the custom ini file directly. We should merge envs and ini files items on memory when gitea start and envs will override ini items. |
@lunny that won't work as the ssh commands serv and hook need to be able to find the config. (Admittedly this is a lot less of a problem now I've moved them but they run in a different environment to that of the main server.) |
We also change the custom ini for lots of other variables: gitea/docker/root/etc/s6/gitea/setup Lines 26 to 44 in b84a251
This pr just adds one more line at the end of that list and allows us for the most part to not have to increase those special cases again. |
@sapk using the env solution you suggest doesn't work - Recall the hooks when run from SSH are not necessarily run in the same environment as the Gitea server command. If you're using environment to update gitea in a docker you need to update the ini file so that those commands work. |
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.
LGTM.
I feel that we could do better and integrate this directly in Gitea at start-up (for more than just docker case) but this is still a big improvement and lay down a good naming policy.
@@ -44,6 +44,8 @@ if [ ! -f ${GITEA_CUSTOM}/conf/app.ini ]; then | |||
SECRET_KEY=${SECRET_KEY:-""} \ | |||
envsubst < /etc/templates/app.ini > ${GITEA_CUSTOM}/conf/app.ini |
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.
Can't we drop the template and for example do ?
SECRET_KEY=${SECRET_KEY:-""}
GITEA__security__SECRET_KEY=${GITEA__security__SECRET_KEY:-"$SECRET_KEY"}
(maybe they can get in one line)
The goal could be to deprecate the old variables and use the new generic only later ?
So that ${GITEA_CUSTOM}/conf/app.ini
is only write once and drop template app.ini
@zeripath I feel more and more than we should drop the openssh compat and go full internal to have complete control and don't have bug report related to external config. But that completely an other subject and maybe not shared across users and maintainers. |
@sapk I think dropping OpenSSH from the default docker is fine - however, preventing Gitea from working with OpenSSH is something I would not support. I wouldn't be happy exposing Gitea's SSH server externally. |
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 don't think we need expose all the ini configuration to envs. Since we will move most to database(or other) so UI could change them, I think we could only expose necessary envs.
Once we expose all of them, removing them will not be easy.
Somehow I don't like how dot is treated, maybe treat it also as single underscore |
@lafriks you can't really just map '_' to '.' in the sections very easily without making the mapping even more complex. The point was to offer a definitive conversion from Environment variables to any ini variable so that the whole ini can be exposed and we would never need to add another variable to @lunny Although I would agree that in general exposing all of the configuration by environment seems excessive, in reality we can't really decide what is relevant for a docker user to change - and it appears that it is completely standard to just use environment variables to change this. (For example @tboerger has a repo https://github.com/dockhippie/gitea which has extensive mapping of Environment variables to the ini.) I guess instead of installing this directly in to our docker - I could just move this and adjust this to create a command in contrib that interested parties could use? |
This contrib command provides a mechanism to allow arbitrary setting of ini values using the environment variable in a more docker standard fashion. Environment variable keys should be structured as: "GITEA__SECTION_NAME__KEY_NAME" Use of the command is explained in the README. Partial fix for go-gitea#350 Closes go-gitea#7287
I can add an option to clear the environment of these special environment variables once they have been placed into the ini - if that would help? |
* Add contrib/environment-to-ini This contrib command provides a mechanism to allow arbitrary setting of ini values using the environment variable in a more docker standard fashion. Environment variable keys should be structured as: "GITEA__SECTION_NAME__KEY_NAME" Use of the command is explained in the README. Partial fix for #350 Closes #7287 * Update contrib/environment-to-ini/environment-to-ini.go Co-Authored-By: 6543 <[email protected]> Co-authored-by: Antoine GIRARD <[email protected]> Co-authored-by: 6543 <[email protected]>
This command will allow docker to adjust arbitrary values in the config ini.
Environment variable keys should be structured as: "GITEA__SECTION_NAME__KEY_NAME"
Fixes #350