-
Notifications
You must be signed in to change notification settings - Fork 978
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
Refactor how node binary is retrieved for functions/extensions. #5422
Conversation
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
5312ed1
to
9be77ff
Compare
c81247a
to
9a9c5e7
Compare
Codecov ReportBase: 56.34% // Head: 56.30% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## Remove_go_discovery_module #5422 +/- ##
==============================================================
- Coverage 56.34% 56.30% -0.04%
==============================================================
Files 313 313
Lines 21198 21208 +10
Branches 4325 4327 +2
==============================================================
- Hits 11943 11942 -1
- Misses 8218 8228 +10
- Partials 1037 1038 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Can you write tests for these new functions too? |
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.
This change makes a lot of sense to me and mostly looks good. However, I get a little worried whenever we start messing with paths in the emulator.
Could you test the following cases:
- Emulate an extension, confirm it executes
- Emulate an extension, confirm it uses the right version of node (even when different from the system version)
Doesn't the extensions emulator test exercise this scenario? Was hoping that it did! |
@taeold Totally forgot we had that test running on CI - it definitely does cover that case |
9be77ff
to
8faa94e
Compare
e7d25e2
to
6063984
Compare
cb26de7
to
f549605
Compare
Today, Functions Emulator make strong assumption that all functions target node runtime and pass around references like
nodeBinary
,nodeMajorVersion
, etc.This PR refactors the logic to abstract away
node
intobin
andruntime
. The logic for choosing the appropriatebin
for the function is lifted toRuntimeDelegate
, same piece of code that holds together othernodejs
runtime specific logic.The refactored required small changes to the extensions emulator - looping in @joehan to see if he has any objections.