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

env var name CAN be any unicode char, not just ASCII #378

Merged
merged 1 commit into from
Mar 29, 2023

Conversation

ndeloof
Copy link
Collaborator

@ndeloof ndeloof commented Mar 27, 2023

make godotenv use runes, not byte to process dot env text file, so we support unicode in variable names

closes #277

@ndeloof ndeloof requested review from milas, laurazard and glours and removed request for milas March 27, 2023 12:14
Copy link
Collaborator

@glours glours left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ndeloof ndeloof force-pushed the Flood-Resistant-mirror-drill branch from 9e29e87 to ae077b5 Compare March 28, 2023 08:15
@ndeloof ndeloof force-pushed the Flood-Resistant-mirror-drill branch from ae077b5 to 71293bc Compare March 28, 2023 09:04
Copy link
Member

@milas milas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻


From Open Group Base Spec:

Environment variable names used by the utilities in the Shell and Utilities volume of POSIX.1-2017 consist solely of uppercase letters, digits, and the ( '_' ) from the characters defined in Portable Character Set and do not begin with a digit. Other characters may be permitted by an implementation; applications shall tolerate the presence of such names.

N.B. "Portable character set" is a subset of ASCII-encodable characters

So, much like we also accept other characters (., -), don't see any reason to not allow full Unicode!

"a": "b",
"a[1]": "c",
"a.propertyKey": "d",
"árvíztűrő-TÜKÖRFÚRÓGÉP": "ÁRVÍZTŰRŐ-tükörfúrógép",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed opportunity for an emoji in a test case 🤌

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh boo, we still check to see if it's considered a letter:

failed to read /Users/milas/dev/repro/unicode/.env: line 1: unexpected character "😂" in variable name

Copy link
Member

@milas milas Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but also...

$ docker compose run echo
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HOSTNAME=5fadb26ba525
TERM=xterm
😂=1
HOME=/root

surprise! we do something different for env vars in YAML 🙈

services:
  echo:
   environment:
     - "😂=1"
   image: alpine
   command: env

@ndeloof
Copy link
Collaborator Author

ndeloof commented Mar 28, 2023

While there's POSIX docs about supported formats, we should align with docker run so that users don't get strange behaviour using compose

$ docker run -e 😂=1 alpine env
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HOSTNAME=78568bfa2fcd
😂=1
HOME=/root

so we have to relax the rule...

@ndeloof ndeloof merged commit 43f1d41 into compose-spec:master Mar 29, 2023
@ndeloof ndeloof deleted the Flood-Resistant-mirror-drill branch March 29, 2023 05:55
@milas
Copy link
Member

milas commented Mar 29, 2023

Agreed re: aligning with docker run! Sorry if my spec quoting was unclear -- it says that for interoperability with POSIX-y tools you should stick to ~ASCII, but that it's not prohibited from using other characters

I think it's more of a warning that lots of those tools were built in the 70s and don't have any concept of UTF-8 😬

But regardless, totally fine -- at the end of the day, if you pass Unicode env var names to your Compose service and it can't handle that...don't do that? 🤷🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for non-ASCII characters in environment variable names
3 participants