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

Environment variables should be capitalized #21021

Open
1 task done
adityapatadia opened this issue Sep 17, 2024 · 9 comments
Open
1 task done

Environment variables should be capitalized #21021

adityapatadia opened this issue Sep 17, 2024 · 9 comments
Labels

Comments

@adityapatadia
Copy link

Issue Summary

When using secrets manager like https://www.doppler.com/ the ENV variables are always enforced to be uppercase. It's always best practice to uppercase env variables to ensure compatibility across different shells.

Steps to Reproduce

Try to set uppercase env variable and it does not work.

Ghost Version

5.94

Node.js Version

20

How did you install Ghost?

Docker

Database type

MySQL 8

Browser & OS version

No response

Relevant log / error output

No response

Code of Conduct

  • I agree to be friendly and polite to people in this repository
@github-actions github-actions bot added the needs:triage [triage] this needs to be triaged by the Ghost team label Sep 17, 2024
Copy link
Contributor

sagzy commented Sep 18, 2024

Hey adityapatadia, thanks for raising the issue.

So that I understand your use case better, can you give an example of an env. variable that you'd like to set to uppercase?

@adityapatadia
Copy link
Author

I meant any config that can be set with ENV variable. Take this example database__connection__password. If this is uppercase, Ghost does not recognise it. Ideally it should be possible.

Copy link
Contributor

sagzy commented Sep 18, 2024

Got it, thank you for clarifying!

@sagzy sagzy added the P4 - Low label Sep 18, 2024 — with Linear
@sagzy sagzy removed the needs:triage [triage] this needs to be triaged by the Ghost team label Sep 18, 2024
@markstos
Copy link
Contributor

I would consider this a bug in Doppler rather than Ghost.

While it's a convention that env vars are upper case, it's not a requirement.

It's always best practice to uppercase env variables to ensure compatibility across different shells.

And this assertion is false. No common shells or tools have a problem with env vars not being upper case.

The format of Ghost environment variables serve a specific purpose to map exactly to the names of valid values in Ghost's config file.

There is already complexity in the system by supporting both the JSON config configuration and the environment variables that match exactly.

Supporting another format of environment variables adds unnecessary complexity.

I suggest filing a bug with Doppler instead. They are failing to support valid environment variables.

@adityapatadia
Copy link
Author

adityapatadia commented Sep 22, 2024

It's really impossible to set JSON config in container environments. ENV variables are the only option. Anyway it's a request to consider this.

@imzedi
Copy link

imzedi commented Sep 24, 2024

@adityapatadia
may be you can try out this in your Dockerfile

    environment:
     database__connection__host: ${DB_HOST} # Use Doppler's uppercase
     database__connection__user: ${DB_USER}

@adityapatadia
Copy link
Author

Sounds like a good idea but problem is doppler injects variables on RUN command and not before. Any idea if that would work in some way?

@ekayZ7875
Copy link

Hello @adityapatadia i would like to work upon this issue.Please assign it to me.

@ErisDS ErisDS added the OSS label Nov 8, 2024 — with Linear
@ErisDS ErisDS removed the P4 - Low label Dec 11, 2024
@ErisDS
Copy link
Member

ErisDS commented Dec 11, 2024

This isn't really a bug, but I do accept it would be more convenient if uppercase env vars were supported.

The full context for why it is not supported lives in the nconf repo here - essentially, their implementation causes a jump in memory usag, which needs to be solved first.

If the issue in nconf is resolved, we can update Ghost and tweak the configuration if needed to support it.

Anyone can work on this - we don't assign issues outside of our regular contributors.

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

No branches or pull requests

6 participants