-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
tools: support environment variables via comments #58186
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
tools: support environment variables via comments #58186
Conversation
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.
JS portion LGTM.
I've already seen a bunch of regressions, changing the method signature may not have been the best decision(I'm trying to avoid reading the test file twice) |
830bb71
to
d674f42
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #58186 +/- ##
==========================================
+ Coverage 90.15% 90.17% +0.02%
==========================================
Files 630 629 -1
Lines 186756 186643 -113
Branches 36648 36651 +3
==========================================
- Hits 168362 168306 -56
+ Misses 11193 11133 -60
- Partials 7201 7204 +3 🚀 New features to boost your workflow:
|
d674f42
to
77e4612
Compare
@@ -0,0 +1,16 @@ | |||
'use strict'; | |||
|
|||
// Environment Variables: A_SET_ENV_VAR=A_SET_ENV_VAR_VALUE B_SET_ENV_VAR=B_SET_ENV_VAR_VALUE |
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.
Just a nit but I'd prefer the more compact: // Env: ...
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.
+1 (@joyeecheung suggested the same name)
I’ll go ahead and update the PR
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.
done!
77e4612
to
dfeec6e
Compare
Landed in 6184730 |
PR-URL: #58186 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Added tests are failing when cherry-picked on v22.x-staging, it will require a manual backport if we want it there |
It should address #58179 by adding support for environment variables via comments.
I've not run the entire test suite, only a subset.
A full CI run will be needed to catch any potential regressions!