Filter gitea-specific variables when running tests

[hramrach]

The test code expects that GITEA_CUSTOM is not set. While there is no reason to set this variable some misguided packagers set it globally in /etc/profile rather than in the service definition.

Fixes: #36042

[wxiaoguang]

It won't really "fix" the problem.

Developers can also run the tests directly via IDE (see the screenshot), it is useful for step-by-step debugging via the debugger.

Then you introduce more surprises for developers: why make test succeeds, but manually running one test fails.

Instead of keeping adding more patches, you can show a warning to developers: there is a pre-defined GITEA_CUSTOM environment variable and it will affect the tests.

Actually, not only GITEA_CUSTOM, there are far more similar environment variables, for example: GITEA_WORK_DIR, GITEA_RUN_MODE, etc

---

For other reviewers: the root problem is that some downstream packages have incorrect behaviors, then the whole system is polluted with a global GITEA_CUSTOM, for example:

https://build.opensuse.org/projects/devel:tools:scm/packages/gitea/files/gitea.profile.sh?expand=1

Since the system is polluted, the fix should be done by the downstream packages, otherwise the abused paths will cause more problems.

[hramrach]

You already have stuff like

GITEA_ROOT="$(CURDIR)" GITEA_CONF=tests/mysql.ini ./integrations.mysql.test

It does not sound like the existing test design sticks to the principle that running tests standalone without the setup done in the makefile works.

[wxiaoguang]

It does not sound like the existing test design sticks to the principle that running tests standalone without the setup done in the makefile works.

That's just your guess.

The tests do run standalone, and it helped to get many bugs fixed (I used the debugger to fix #35946, #35899, #35858, #35797, and more)

[silverwind]

We do need to ensure that tests run in a controlled environment. As a more general solution, I'd recommend we add a dotenv file like test.env and make sure this file is loaded before every test, and it has to work with go test too, e.g. no Makefile involved.

https://github.com/joho/godotenv can be used to load dotenv files.

[hramrach]

Why does the makefile pass environment variables to that test if it's supposed to run standalone then?

[wxiaoguang]

Yes, that's why using environment variables fro configuration is ReallyBadIdea™.

Using environment variables for configuration isn't really bad, many modern libraries also support it, and git also support it.

Polluting user's environment variables is the real bad idea.

[wxiaoguang]

Downstream can use -X "setting.CustomPath=/etc/gitea" for Makefile's LDFLAGS

Then the binary will use the correct path and config without any global variable.

---

If downstream only want to pack existing binary, they can use Gitea's docker's wrapper script: https://github.com/go-gitea/gitea/blob/main/docker/root/usr/local/bin/gitea

Then no global variable is needed.

---

These 2 are the proper fixes. The wrapper script is recommended, it doesn't need to use any special build

[hramrach]

You enumerate the reasons why it's bad and then say it's not really bad, and argue others are doing it.

No, it is bad to use environment variables for configuration. It is obfuscation of configuration, a way to slip some configuration behind the user's back.

If environment variables were not used in the code we would not have these discussions, there would be nothing to pollute.

And it's still very much possible to use make variables to pass in configuration even if the code under test does not use environment variables.

And when not using make it's possible to pass commandline arguments. A thing that it explicit, visible, and cannot be slipped into a file behind the user's back.

That's not to say that gitea should stop using environment variables for configuration overnight or even at all. Change is hard, and takes a long time when attempted.

[TheFox0x7]

Isn't the solution the most basic part of tests? Clean environment? Which CI, clean VM or container generally provides?

Funny thing about env vars and git being relevant - you know what tests fail on my local setup? Yup. Git. Specifically XDG_CONFIG_HOME is set so config edits default to that. Never bothered to report it or look into it as I know it's fixed in the proper binary and running XDG_CONFIG_HOME= make test-backend isn't really a bother. I find it ironic considering this entire thread :)

No, it is bad to use environment variables for configuration. It is obfuscation of configuration, a way to slip some configuration behind the user's back.

I think you should tell that to users of kubernetes and docker/podman. And people from https://12factor.net/config. And after all that deal with users of gitea/any other app explaining that it's not a regression and they have to rewrite their config to use file instead of env vars.

[wxiaoguang]

Maybe you should also convince other projects that "environment variables are bad":

• Let Golang drop their GOPROXY
• Let Docker drop their DOCKER_BUILDKIT
• Let git drop their GIT_COMMITTER_NAME
• Let Zypper drop their ZYPP_CONF

OK, I have explained everything.

• Why your change doesn't really fix:
  • Filter gitea-specific variables when running tests #36070 (comment)
  • Filter gitea-specific variables when running tests #36070 (comment)
• How to properly fix: Filter gitea-specific variables when running tests #36070 (comment)
• (Update): How to handle env vars: Filter gitea-specific variables when running tests #36070 (comment)

I will unsubscribe this thread to save everyone's time. As always: you can choose to believe or not, and do the things that you think are right.