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

Document environment variables #5735

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Manas-Kenge
Copy link

@Manas-Kenge Manas-Kenge commented Feb 5, 2024

Fixes #4581 #5319

@tiberiuichim I updated the docs as per my understanding, was I supposed to create a separate section for it? I looked at the Razzle docs for reference.

Copy link

netlify bot commented Feb 5, 2024

Deploy Preview for volto ready!

Name Link
🔨 Latest commit 7bd7ce8
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/65ecbb2eeb260c0008a9a67e
😎 Deploy Preview https://deploy-preview-5735--volto.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Feb 5, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit 7bd7ce8
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/65ecbb2e1eb9c900075d0dcb

@stevepiercy
Copy link
Collaborator

@Manas-Kenge please hold on this pull request, per #4581 (comment).

@Manas-Kenge
Copy link
Author

Ok, no problem.

@stevepiercy
Copy link
Collaborator

@Manas-Kenge I overhauled the file in #5736. You may rebase your work on main and resume work on this issue.

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Mostly restructure as a glossary, following the existing pattern in this file.

Also a couple of questions that require a maintainer to answer.

packages/volto/news/5319.documentation Outdated Show resolved Hide resolved
`process.env.RAZZLE_INTERNAL_API_PATH`: Used to specify the path to an internal API that the server-rendered application should use.

`process.env.RAZZLE_PROXY_REWRITE_TARGET`: Used to specify the target URL for a proxy server.

Copy link
Collaborator

Choose a reason for hiding this comment

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

All of these environment variables should follow the existing pattern as the glossary items below.

I also don't know whether the prefix process.env. is necessary. Perhaps a Volto core maintainer knows?

Finally, do these belong under this heading or another? Again, perhaps a Volto core maintainer knows?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

@stevepiercy @Manas-Kenge process.env. is not part of the name of the environment variable. It's how the variable is accessed in Node. It doesn't make sense to use it here, which is explaining variables that can be set, not how to write code that uses them.

docs/source/configuration/environmentvariables.md Outdated Show resolved Hide resolved
docs/source/configuration/environmentvariables.md Outdated Show resolved Hide resolved
Manas-Kenge and others added 2 commits March 9, 2024 20:40
@Manas-Kenge
Copy link
Author

I referred the Razzle documentation https://razzlejs.org/docs/environment-variables for this.
Should I just remove process.env then?

@davisagli
Copy link
Sponsor Member

@Manas-Kenge The Razzle documentation is describing environment variables that are set automatically by Razzle and can be accessed by application code.

Our docs are trying to describe environment variables that can be set manually from outside the application to influence how it runs.

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

I pushed a commit to convert the items to a Glossary. LGTM, but it needs a final review from a maintainer.

@stevepiercy stevepiercy dismissed their stale review March 9, 2024 19:41

docs format looks good, content needs review for accuracy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

Environment variables need to be properly documented
3 participants