Skip to content
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

Closed
wants to merge 21 commits into from

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Jun 24, 2019

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

@codecov-io
Copy link

codecov-io commented Jun 24, 2019

Codecov Report

Merging #7287 into master will increase coverage by 0.01%.
The diff coverage is 48.86%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
cmd/environment_to_ini.go 48.86% <48.86%> (ø)
models/unit.go 62.16% <0%> (-5.41%) ⬇️
modules/process/manager.go 78.37% <0%> (-4.06%) ⬇️
modules/log/event.go 64.61% <0%> (-1.03%) ⬇️
models/repo_indexer.go 68.36% <0%> (+1.81%) ⬆️
modules/indexer/indexer.go 55.26% <0%> (+10.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2b9bda...bd44265. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 24, 2019
@zeripath
Copy link
Contributor Author

@christhomas is this sufficient to map everything else? Would the colon separator be too difficult to work with?

@christhomas
Copy link
Contributor

christhomas commented Jun 24, 2019

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.

export -p GITEA:TEST:VAR=hello
-bash: export: `GITEA:TEST:VAR=hello': not a valid identifier

I must have misunderstood you? Cause I think you must be talking about something else

@zeripath
Copy link
Contributor Author

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?

cmd/environment_to_ini.go Outdated Show resolved Hide resolved
cmd/environment_to_ini.go Outdated Show resolved Hide resolved
@zeripath
Copy link
Contributor Author

zeripath commented Jun 24, 2019

Given that we're likely to be stuck with keys only having the portable set - we do need some way of representing . - could add an escaped format eg _0x2e_ to allow for non portable characters.

@christhomas
Copy link
Contributor

Do you have an example of a non portable character?

Why would you want to represent ',' inside a key of a kv store?

@zeripath
Copy link
Contributor Author

If you peruse the linked stack overflow comment you'll see that they define portable as [a-zA-Z_][a-zA-Z_0-9]*.

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.

@christhomas
Copy link
Contributor

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.

@zeripath
Copy link
Contributor Author

Ah but we do need the period - see the configuration of subloggers or cron. There's also [repository.pull-request] so we need - too. (Yes I can't imagine why people would want to configure those this way but if we're going to do this let's do it properly.)

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.)

@zeripath
Copy link
Contributor Author

zeripath commented Jun 26, 2019

OK done. We now have a completely generalised way of setting via environment variables.

@zeripath zeripath requested a review from tboerger June 30, 2019 17:46
@lunny
Copy link
Member

lunny commented Jul 11, 2019

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.

@zeripath
Copy link
Contributor Author

zeripath commented Jul 11, 2019

@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.)

@zeripath
Copy link
Contributor Author

zeripath commented Jul 11, 2019

We also change the custom ini for lots of other variables:

# Substitude the environment variables in the template
APP_NAME=${APP_NAME:-"Gitea: Git with a cup of tea"} \
RUN_MODE=${RUN_MODE:-"dev"} \
SSH_DOMAIN=${SSH_DOMAIN:-"localhost"} \
HTTP_PORT=${HTTP_PORT:-"3000"} \
ROOT_URL=${ROOT_URL:-""} \
DISABLE_SSH=${DISABLE_SSH:-"false"} \
SSH_PORT=${SSH_PORT:-"22"} \
LFS_START_SERVER=${LFS_START_SERVER:-"false"} \
DB_TYPE=${DB_TYPE:-"sqlite3"} \
DB_HOST=${DB_HOST:-"localhost:3306"} \
DB_NAME=${DB_NAME:-"gitea"} \
DB_USER=${DB_USER:-"root"} \
DB_PASSWD=${DB_PASSWD:-""} \
INSTALL_LOCK=${INSTALL_LOCK:-"false"} \
DISABLE_REGISTRATION=${DISABLE_REGISTRATION:-"false"} \
REQUIRE_SIGNIN_VIEW=${REQUIRE_SIGNIN_VIEW:-"false"} \
SECRET_KEY=${SECRET_KEY:-""} \
envsubst < /etc/templates/app.ini > ${GITEA_CUSTOM}/conf/app.ini

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.

@zeripath
Copy link
Contributor Author

zeripath commented Oct 9, 2019

@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.

@zeripath zeripath requested a review from sapk October 17, 2019 13:27
Copy link
Member

@sapk sapk left a 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
Copy link
Member

@sapk sapk Oct 19, 2019

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

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 19, 2019
@sapk
Copy link
Member

sapk commented Oct 19, 2019

@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.

@zeripath
Copy link
Contributor Author

zeripath commented Nov 3, 2019

@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.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 22, 2019
Copy link
Member

@lunny lunny left a 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.

@lafriks
Copy link
Member

lafriks commented Dec 24, 2019

Somehow I don't like how dot is treated, maybe treat it also as single underscore

@zeripath
Copy link
Contributor Author

@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 docker/root/etc/templates/app.ini ever again. The problem would be solved forever.

@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?

zeripath added a commit to zeripath/gitea that referenced this pull request Dec 27, 2019
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
@zeripath
Copy link
Contributor Author

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?

@lunny lunny closed this in #9519 Dec 28, 2019
lunny pushed a commit that referenced this pull request Dec 28, 2019
* 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 <6543@obermui.de>

Co-authored-by: Antoine GIRARD <sapk@users.noreply.github.com>
Co-authored-by: 6543 <6543@obermui.de>
@zeripath zeripath mentioned this pull request Mar 28, 2020
5 tasks
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@zeripath zeripath deleted the environment-to-ini branch March 4, 2021 09:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configure with env variables
9 participants