styra-link: unhandled bad download URL for styra CLI causes cryptic error #78
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
msorens
commented
Apr 16, 2024
Comment on lines
16
to
+18
| if (!await StyraInstall.styraCmdExists()) { | ||
| info(`"${STYRA_CLI_CMD}" not found; aborting ${command.title}`); | ||
| return; |
There was a problem hiding this comment.
PR bonus: OK, let's abort then!
Comment on lines
102
to
-103
| if (!StyraInstall.checkWorkspace()) { | ||
| teeError('Something is wrong! Did you forget to run checkWorkspace in your command?'); |
There was a problem hiding this comment.
PR bonus: Not clear why this would ever have been useful, as the only way this appears is if we are running checkWorkspace in the prior line. 😱
Comment on lines
-28
to
+32
| StyraInstall.styraCmdExists = jest.fn().mockResolvedValue(true); | ||
| IDE.getConfigValue = mockVSCodeSettings(); | ||
|
|
||
| beforeEach(() => { | ||
| StyraInstall.styraCmdExists = jest.fn().mockResolvedValue(true); | ||
| }); |
There was a problem hiding this comment.
PR bonus: turns out that the PR bonus above that really did abort when it claimed to exposed an error in the test; this line needed to be in the beforeEach block for the tests to pass.
srenatus
reviewed
Apr 16, 2024
msorens
force-pushed
the
msorens/bad-url
branch
from
eec1d5e
to
f88eedc
Compare
msorens
force-pushed
the
msorens/optimize-package
branch
from
794c661
to
a19caf5
Compare
With a bad URL the download still resulted in something, (seemed to be a web page indicating 404) so the code blithely installed that as /usr/local/bin/styra. With that in place, trying to invoke it as an executable resulted in the cryptic error "Error spawn Unknown system error -8". This change now recognizes that the non-empty download was not an executable, so fails the "install" operation properly.
not clear why this was ever useful
Getting here would be very rare, though, if even possible at all, because the checkStartup before would ensure CLI installation.
msorens
force-pushed
the
msorens/bad-url
branch
from
f88eedc
to
f817a83
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What code changed, and why?
With a bad URL the automatic install of the Styra CLI by the Styra VSCode plugin still resulted in storing something at /usr/local/bin/styra, but not an executable binary(!). (Looks like a 404 page, essentially.) The code assumed that was the real thing, reported the CLI installation successful(!), tried to invoke it as the styra CLI, and said "Error spawn Unknown system error -8", as shown:
This PR will now let the plugin recognize that the non-empty download was not an executable, so fails the "install" operation properly.
The solution here is not terribly elegant—just seeing if the download was larger than one megabyte—but it is a fast check.
I opted for fast rather than thorough because something like this would happen only rarely (like, um, well, now). 🤷
Again, because it is rare, this handles the case of /usr/local/bin/styra being an imposter at install time only. The other case—which it does not handle—is if you already have an imposter /usr/local/bin/styra when you try to run
Styra Link: Initialize...Definition of done
With the current bad URLs for downloading the styra CLI, the system will now recognize the attempted install of /usr/local/bin/styra failed and abort at that point.
2024.04.16 update:
Using the HTTP response instead of my initial crude approach; also adjusted the output here slightly:
How to test
You can install either from source code or from GitHub.
Install from source
npm install.Install from GitHub
vscode-styra-2.0.1-next.2.vsix.code --install-extension vscode-styra-2.0.1-next.2.vsixExercise the Code
Make sure you do not have
styraon your search path, and in particular that you do not have it in /usr/local/bin, since that is where the plugin installs to.Attempt to run the VSCode palette command
Styra Link: Initialize....Say "Yes" when asked if you want to install the Styra CLI.
Observe the Output pane.