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

[cli] Change vc env pull default output file to .env.local #9892

Merged
merged 13 commits into from
May 22, 2023

Conversation

dan-stowell
Copy link
Contributor

vc deploy ignores .env.local. To make sure we don't inadvertently push people's secrets to source control, have all environment pulls default to writing to .env.local.

Linear ticket

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Does vc dev need to be updated to read .env.local?

Looks like we decided that vc dev should be changed to stop reading .env entirely.

@styfle styfle changed the title Have pull write to .env.local [cli] MAJOR: vc env pull default to .env.local May 2, 2023
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Don't forget to update --help as well as vercel.com/docs

@dan-stowell
Copy link
Contributor Author

@styfle does MAJOR mean we need to bump the major version for this change?

@styfle
Copy link
Member

styfle commented May 2, 2023

@dan-stowell Yes this is semver-major because its a breaking change (needs the label too)

@dan-stowell dan-stowell added area: cli semver: major PR contains breaking changes labels May 2, 2023
cb1kenobi
cb1kenobi previously approved these changes May 3, 2023
Copy link
Contributor

@cb1kenobi cb1kenobi left a comment

Choose a reason for hiding this comment

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

Tested and works as advertised.

Copy link
Contributor

@EndangeredMassa EndangeredMassa left a comment

Choose a reason for hiding this comment

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

Should we update these references to .env as well?

  • errors/missing-env-file.md - should refer to .env.local?
  • examples/sanity/.env.template - should be .env.local.template?
  • packages/cli/src/util/dev/server.ts - line 674 and 675 should use .env.local and maybe fall back to .env?
  • packages/cli/src/util/dev/types.ts - comments in EnvConfigs should refer to .env.local

I'm not positive how best to update dev/server.ts, but I think we should take a stab at them in this PR.

@dan-stowell
Copy link
Contributor Author

Should we update these references to .env as well?

  • errors/missing-env-file.md - should refer to .env.local?

Done & pushed.

  • examples/sanity/.env.template - should be .env.local.template?

Done & pushed.

  • packages/cli/src/util/dev/server.ts - line 674 and 675 should use .env.local and maybe fall back to .env?
  • packages/cli/src/util/dev/types.ts - comments in EnvConfigs should refer to .env.local

Based on discussion in Linear, it seems that we do not want vc dev to read from .env at all. If that's the case, I'd like to make that change in a separate pull request.

@dan-stowell dan-stowell dismissed EndangeredMassa’s stale review May 4, 2023 19:30

Let's make vc dev changes separately.

styfle
styfle previously approved these changes May 4, 2023
EndangeredMassa
EndangeredMassa previously approved these changes May 5, 2023
cb1kenobi
cb1kenobi previously approved these changes May 5, 2023
Copy link
Contributor

@cb1kenobi cb1kenobi left a comment

Choose a reason for hiding this comment

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

LGTM

@cb1kenobi cb1kenobi added pr: automerge Automatically merge the PR when checks pass and removed pr: automerge Automatically merge the PR when checks pass labels May 8, 2023
@TooTallNate TooTallNate changed the title [cli] MAJOR: vc env pull default to .env.local [cli] Change vc env pull default output file to .env.local May 22, 2023
@TooTallNate TooTallNate added the pr: automerge Automatically merge the PR when checks pass label May 22, 2023
@changeset-bot
Copy link

changeset-bot bot commented May 22, 2023

🦋 Changeset detected

Latest commit: c33be8c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
vercel Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@TooTallNate TooTallNate merged commit 1b0d72a into main May 22, 2023
102 checks passed
@TooTallNate TooTallNate deleted the ds-env-local branch May 22, 2023 22:24
kodiakhq bot pushed a commit that referenced this pull request Jun 8, 2023
This is a follow up to PR #9892 which changed the default to `.env.local`.

Now that we know local files should never be committed to git, we can automatically add `.env*.local` to `.gitignore`. Note that this is the same ignore pattern that ships with create-next-app [as seen here](https://github.com/vercel/next.js/blob/06abd634899095b6cc28e6e8315b1e8b9c8df939/packages/create-next-app/templates/app/js/gitignore#L28), so most Next.js users won't see anything change.

See the related [Linear ticket](https://linear.app/vercel/issue/VCCLI-461/)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: cli pr: automerge Automatically merge the PR when checks pass semver: major PR contains breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants