Skip embedded Node.js runtime deployment if sonar.nodejs.executable is set
#4616
Conversation
ilia-kebets-sonarsource
requested review from
vdiez
and removed request for
vdiez
ilia-kebets-sonarsource
requested review from
vdiez
and removed request for
vdiez
ilia-kebets-sonarsource
changed the title
sonar.nodejs.executable is set
.../sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/EmbeddedNode.java
Outdated
Show resolved
Hide resolved
.../sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/EmbeddedNode.java
Outdated
Show resolved
Hide resolved
...avascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/BridgeServerImplTest.java
Outdated
Show resolved
Hide resolved
...ar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/BridgeServerImpl.java
Outdated
Show resolved
Hide resolved
.../sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/EmbeddedNode.java
Outdated
Show resolved
Hide resolved
...avascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/BridgeServerImplTest.java
Outdated
Show resolved
Hide resolved
...avascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/BridgeServerImplTest.java
Show resolved
Hide resolved
| var existingDoesntMatterScript = "logging.js"; | ||
| bridgeServer = createBridgeServer(existingDoesntMatterScript); | ||
| context.setSettings(new MapSettings().setProperty(NODE_EXECUTABLE_PROPERTY, "whatever")); | ||
| assertThatThrownBy(() -> bridgeServer.startServerLazily(context)) | ||
| .isInstanceOf(NodeCommandException.class); | ||
|
|
||
| assertThat(logTester.logs(INFO)) | ||
| .contains( | ||
| "'" + NODE_EXECUTABLE_PROPERTY + "' is set. Skipping embedded Node.js runtime deployment." | ||
| ); | ||
| } |
There was a problem hiding this comment.
Before this, my idea was to run bridgeServer.startLazily() twice. First to extract a node runtime that would be used by the second call to it.
This requires quite a lot of boilerplate code since the helper functions in BridgeServerImplTest.java have hardcoded an unsupported platform for the BridgeServer EmbeddedNode instance.
Since I don't have an embedded runtime, it would require me to set a node executable depending on the platform, which is also cumbersome.
.../sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/EmbeddedNode.java
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Huh, surprising that the code review still requires me specifically to review even if there is someone else who accepted. I'm used to only need to have a single accept, without counting how many request changes were there before, sorry for the wait.
I include a question about the FORCE_HOST flag and a small suggestion about the new test.
| bundle.deploy(temporaryDeployLocation); | ||
| if (configuration.get(NODE_EXECUTABLE_PROPERTY).isPresent()) { |
There was a problem hiding this comment.
unrelated to your changes, but I assume this code is unreachable (this deploy method) if the user set the FORCE_HOST flag?
| var existingDoesntMatterScript = "logging.js"; | ||
| bridgeServer = createBridgeServer(existingDoesntMatterScript); | ||
| context.setSettings(new MapSettings().setProperty(NODE_EXECUTABLE_PROPERTY, "whatever")); | ||
| assertThatThrownBy(() -> bridgeServer.startServerLazily(context)) |
There was a problem hiding this comment.
really funky that this would throw an error. This seems like it is testing the current behavior (as in, implementation detail) and not the actual functionality we want, which should be: "We will not try to load the Embedded node and instead only try the provided path".
I would imagine that the underlying implementation could change so that the server starts without any exception and correctly runs the provided path, and then this test would fail and be a false positive, by this test failing.
I do like the latter log check, as it shows this functionality you are adding. Perhaps we wouldn't assert that it throws above, just inspect the NodeCommand(?) doesn't try to access the embedded node?
ilia-kebets-sonarsource
deleted the
i4400-do-not-deploy-runtime-if-sonar.nodejs.executable-is-set
branch
I think we could configure it, maybe add a point to the weekly meeting to discuss it |
Fixes #4400