server panics with incorrect WOODPECKER_ENVIRONMENT

[wxiaoguang]

Component

server

Describe the bug

Bug

The expected WOODPECKER_ENVIRONMENT is:

WOODPECKER_ENVIRONMENT=k:v,foo:bar

But if we write:

WOODPECKER_ENVIRONMENT=k:v,foo=bar

Then the server panics.

Syntax

The syntax WOODPECKER_ENVIRONMENT=k:v,foo:bar is very difficult to read or write, especially when there are a lot of variables, or some variables contain comma or colon.

I think the new syntax (use 2 underlines) can be better:

WOODPECKER_ENV__X=y
WOODPECKER_ENV__FOO=bar

Log

panic: runtime error: index out of range [1] with length 1

goroutine 1 [running]:
github.com/woodpecker-ci/woodpecker/server/plugins/environments.Filesystem(0xc0004cd780, 0x4, 0x4, 0xc0004cd780, 0x4)
        /woodpecker/src/github.com/woodpecker-ci/woodpecker/server/plugins/environments/filesystem.go:19 +0x2a8
main.setupEnvironService(0xc000240c80, 0x142d6c8, 0xc000194430, 0x7f2b240a3140, 0xc000194430)
        /woodpecker/src/github.com/woodpecker-ci/woodpecker/cmd/server/setup.go:176 +0x67
main.setupEvilGlobals(0xc000240c80, 0x142d6c8, 0xc000194430, 0x1425300, 0xc0006aa180)
        /woodpecker/src/github.com/woodpecker-ci/woodpecker/cmd/server/server.go:273 +0x45c
main.run(0xc000240c80, 0x0, 0x0)
        /woodpecker/src/github.com/woodpecker-ci/woodpecker/cmd/server/server.go:122 +0x413
github.com/urfave/cli/v2.(*App).RunContext(0xc0001a0680, 0x1418a68, 0xc000044040, 0xc000034220, 0x1, 0x1, 0x0, 0x0)
        /woodpecker/src/github.com/woodpecker-ci/woodpecker/vendor/github.com/urfave/cli/v2/app.go:322 +0x6fe
github.com/urfave/cli/v2.(*App).Run(...)
        /woodpecker/src/github.com/woodpecker-ci/woodpecker/vendor/github.com/urfave/cli/v2/app.go:224
main.main()
        /woodpecker/src/github.com/woodpecker-ci/woodpecker/cmd/server/main.go:35 +0x134

System Info

Latest next.

"version": "next-8ce41788"

Additional context

No response

Validations

• [X] Read the Contributing Guidelines.
• [X] Read the docs.
• [X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• [X] Check that this is a concrete bug. For Q&A join our Discord Chat Server.

[6543]

looks like an issue of urfave/cli - but I'll have a look at it

[wxiaoguang]

How do you think about using this syntax?

WOODPECKER_ENV__X=y
WOODPECKER_ENV__foo=bar

[6543]

well I would fix it upstream ... (It's indeed a bug of that lib)

[wxiaoguang]

If I propose a PR about the feature of WOODPECKER_ENV__key, is it acceptable?

In many cases, WOODPECKER_ENVIRONMENT=k:v,foo:bar is more difficult to use, for example, if there are many environment variables, if the key/value are long, if there are commas or colons in values, etc. Using WOODPECKER_ENV__key=value would make it easier and more clear for users.

[6543]

well it would definetly add extra code ... and I dont see much upside beside more redable docker-compose configs ...

what do you want to store in env vars, as you argue about to long key/value ?!? you can store a lot in it...

[6543]

@wxiaoguang the thing is, in my opinion if you run out of space for the information you like to pass on to, via environment-vars, you probably try to solve the problem the wrong way ...

[wxiaoguang]

well it would definetly add extra code ...

Hmm ... just a few lines, a for loop.

and I dont see much upside beside more redable docker-compose configs ... what do you want to store in env vars, as you argue about to long key/value ?!? you can store a lot in it...

Now I use WOODPECKER_ENVIRONMENT=DOCKER_HOST:unix:///data-dind/dockerd/docker.sock,DOCKER_CONFIG:/data-dind/docker-config,.... to pass common variables to all agents. I think here is the correct place to write these variables, instead of writing them in every agent docker-compose file.

And I can not use commas in the variable value, it breaks the syntax ......

---

And yes, the feature WOODPECKER_ENV__key is not a must, it could always be rewritten into separate agent docker-compose files.

[6543]

-> #781

[6543]

@wxiaoguang as opt in for v1.0.0 we can argue about, If it indeed is only a few lines of code ... I will have my thoughts about this again once at least a code draft exists we can talk about - for now as a bugfix this is not wat we should talk about ... beside I just found the dumb mistake made already ...

tldr: it would be nice if new idears would be discusted in new issues, of course they can&should be referenced to where they originated from ....

[wxiaoguang]

No problem. Thank you very much.

ps: should the code output an error log if len(kvPair) != 2? it could help users to debug their mistakes more easliy.