-
Notifications
You must be signed in to change notification settings - Fork 343
chore(meta): add bootstrap.yaml and update .env for bootstrap #298
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
Conversation
|
@rektdeckard is attempting to deploy a commit to the dsalivekitio's projects Team on Vercel. A member of the Team first needs to authorize it. |
| LIVEKIT_API_SECRET=secret | ||
| # API key and secret. If you use LiveKit Cloud this can be generated via the cloud dashboard, | ||
| # or by running `lk cloud auth` with the LiveKit CLI. | ||
| LIVEKIT_API_KEY="$LIVEKIT_API_URL" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand this change, this is an example .env file trying to help users to fill out the correct env vars.
I don't think we should bake any internal-tooling assumptions in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably makes a little more sense in context with this PR which implements the bootstrap commands.
The CLI will clone down a template repo, copy any .env.example to .env.local, replacing known placeholder values with real values. If your issue is with the magic string substitution, I'm sure we could come up with a solution that simply parses the env file for the presence of keys, not values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my head it's mainly about .env.example being a template for users to set up their own env file.
Using variable placeholders doesn't really help users in that regard and might rather be potentially confusing to them.
We could of course add comments outlining the expected values, but if the approach I suggested in the other comment (using the bootstrap.yaml for env var mapping) seems reasonable to you, I'd (purely personally) prefer not mixing in things in the example files.
| install: | ||
| - "pnpm install" | ||
| dev: | ||
| - "pnpm dev" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if the bootstrap.yaml approach has been agreed upon, but I think it would make more sense to have the env var mapping in here rather than altering example env files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would work. It's more repetitive for projects with multiple components (say, a client and server directory written in different languages) but probably overall better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it have to be repetitive if the env key was a root-level mapping that could be used by different (all) components?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My new thinking here is that we actually use task as an embedded task runner, and use their taskfile.yaml format out of the box. It comes with pretty awesome features for working with environment and lets you write bash script syntax even for windows targets.
Prepares this repo for use as a playground / bootstrap template, per spec discussion.