Add ability to declare a env key in each pipeline task #1932
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
9bfc0ed to
a765d99
Compare
13d57ed to
d4c2285
Compare
d4c2285 to
fb3f942
Compare
0b192b9 to
90b743f
Compare
90b743f to
8003fb0
Compare
548be53 to
9b4909d
Compare
|
Reverting docs out of here, so we can wait until a release to update the docs. See #1936 for the changes. |
nathanhammond
left a comment
There was a problem hiding this comment.
There is also docs/schema.d.ts which needs to be updated. Was that part reverted out?
I moved to #1936 since we don't want to update it until a release! (my understanding is that schema is used only for the |
cli/internal/fs/turbo_json.go
Outdated
There was a problem hiding this comment.
I wouldn't include this warning. In general, people ignore warnings unless something breaks. If something breaks, they'll investigate. When something does break, we don't know that it will be related to this, or any other warning that we might include. In the worst case, we send them on a false trail because whatever broke is unrelated. The best help we can give them is to show what commands were invoked with what inputs. This is where something like a run summary comes into play.
For this warning in particular, users of this API will fall into two camps:
- Users coming from the previous behavior of
dependsOn. - Users new to
turbowho have never seen the previous syntax
We expect going forward that group 2 will be the much larger group. They will have no reason to think they need a $ prefix. In the event of an unlikely subset who do intend to set an environment variable that starts with a $, they will then have a warning they can't get rid of.
Group 1 will have a codemod to move them to the proper syntax.
There was a problem hiding this comment.
Also, if we do end up keeping this warning, we need to thread our cli.Ui instance in here, so that it can respect whatever output is configured. We shouldn't be using log directly.
There was a problem hiding this comment.
ok, that's fair too. I've removed the warning
There was a problem hiding this comment.
@gsoltis the issue here is that somebody will add "$FOO" to the env, get no feedback, and just assume that it works.
Later, when their cache isn't segmented, they'll have no idea why and (at this time) have no way to inspect the build to understand what happened.
In other words, failing to use this API correctly will ruin your day. We should aim to reduce the incidence of these types of failure modes which is why I propose automatically removing the $.
There was a problem hiding this comment.
Reached consensus in slack to throw a hard error for $ prefixed strings
|
I tested this locally with my |
nathanhammond
left a comment
There was a problem hiding this comment.
Some proposed verbiage changes.
cli/internal/fs/turbo_json_test.go
Outdated
There was a problem hiding this comment.
This is fine for this case but we should (all) start to get into the habit of fmt.Errorf(%w, err) so that we can use error.Is.
1edc7dd to
db4736f
Compare
Adds the ability to read environment variable dependencies for each task from an
envkey. It supports both configs, but logs a deprecation message on the old config, and de-dupes. The de-duping feature also applies duplicate declarations in the existingdependsOnkey. (Not sure if this was being de-duped somewhere downstream already)Coming up after this PR:
(See #1929 for some initial refactor work that added a few more test cases if interested!)