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

[docs] Warn/clarify that env vars are NOT "SECRET" #6062

Merged
merged 3 commits into from
Feb 10, 2019
Merged

[docs] Warn/clarify that env vars are NOT "SECRET" #6062

merged 3 commits into from
Feb 10, 2019

Conversation

JBallin
Copy link
Contributor

@JBallin JBallin commented Dec 18, 2018

Fixes #5676

@JBallin
Copy link
Contributor Author

JBallin commented Dec 28, 2018

Lines 26-30 (in my branch, pasted below) need to be adjusted as well, but I would like some clarity. I'm uncertain why there are such deliberate assertions that secrets can be used in env vars, am I misunderstanding something? I would also like some insight into when not to include env vars in version control.

These environment variables can be useful for displaying information conditionally based on where the project is deployed or consuming sensitive data that lives outside of version control.

First, you need to have environment variables defined. For example, let’s say you wanted to consume a secret defined in the environment inside a <form>:

@petetnt
Copy link
Contributor

petetnt commented Dec 30, 2018

First, you need to have environment variables defined. For example, let’s say you wanted to consume a secret defined in the environment inside a

:

I think the secret there can be safely changed to "variable" there.

Copy link
Contributor

@petetnt petetnt left a comment

Choose a reason for hiding this comment

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

Few tiny comments, all in all I think this is a pretty good change and something that seems to trip many new CRA users (including people in my team).

docusaurus/docs/adding-custom-environment-variables.md Outdated Show resolved Hide resolved
docusaurus/docs/adding-custom-environment-variables.md Outdated Show resolved Hide resolved
@JBallin
Copy link
Contributor Author

JBallin commented Jan 6, 2019

Thanks for the review @petetnt, incorporated your suggestions. Still unsure about this line:

These environment variables can be useful for displaying information conditionally based on where the project is deployed or consuming sensitive data that lives outside of version control.

@petetnt
Copy link
Contributor

petetnt commented Jan 6, 2019

I think that the whole sentence could be removed, it doesn't offer much / anything that wasn't explained in the document beforehand.

@JBallin
Copy link
Contributor Author

JBallin commented Jan 7, 2019

I think that the whole sentence could be removed, it doesn't offer much / anything that wasn't explained in the document beforehand.

@petetnt I actually like the first part of the sentence (up until "deployed"), but I'm still curious as to why version control was brought up.

These environment variables can be useful for displaying information conditionally based on where the project is deployed or consuming sensitive data that lives outside of version control.

@stale
Copy link

stale bot commented Feb 7, 2019

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Feb 7, 2019
@JBallin
Copy link
Contributor Author

JBallin commented Feb 9, 2019

I’m still waiting for a response from the maintainers, so this should be kept open.

@stale stale bot removed the stale label Feb 9, 2019
Copy link
Contributor

@ianschmitz ianschmitz left a comment

Choose a reason for hiding this comment

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

Thanks for this @JBallin. Just one small tweak and I think we should be good.

docusaurus/docs/adding-custom-environment-variables.md Outdated Show resolved Hide resolved
@ianschmitz ianschmitz added this to the 2.1.4 milestone Feb 10, 2019
@ianschmitz ianschmitz merged commit 313e472 into facebook:master Feb 10, 2019
@ianschmitz
Copy link
Contributor

Thanks @JBallin!

@JBallin
Copy link
Contributor Author

JBallin commented Feb 10, 2019

Thank YOU @ianschmitz, I'm so excited to have made my first contribution to CRA!

Unfortunately, an unintended change snuck through. I've opened a PR (#6386) to fix it. Sorry about that.

@lock lock bot locked and limited conversation to collaborators Feb 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants