This repository was archived by the owner on May 15, 2025. It is now read-only.
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.
feat(vscode-web): add offline, use_cached, extensions_dir and auto_install_extensions #235
feat(vscode-web): add offline, use_cached, extensions_dir and auto_install_extensions #235
Changes from all commits
f908e45
23ff3ca
27a1def
dcc1f66
d509b81
1d9585c
5685613
b78593c
c6857f7
cc614e7
b8ebdc9
1462d66
a367517
9bb4b82
b74a61c
b00b47c
0605dce
3c0c133
4c016da
2d43c2c
ffe9114
2d15e4b
10f9f15
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 occurred to me, should this be considered a failure?
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.
I am not sure. From a user perspective it would be nice to continue without an error and print a warning
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.
I guess there is where monitoring works when errors happen in a large deployment.
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.
Oh if
exit 1
prevents the workspace from starting then yeah, better to continue.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.
it doesn't prevent the workspace from starting but does show a warning on the page that the script was not successful.
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.
Ahh gotcha yeah
exit 1
seems like the right move then.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.
I think it's fine. If we don't fine jw we should just skip installing extensions and continue with the rest of the script.
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.
I have mixed feelings about it, because enabling autoinstall without
jq
is a bug the template author has to fix (I think, right?), and not showing the error could make it so no one notices.On the other hand, it does seem annoying as a user that I cannot launch VS Code just because someone forgot to install
jq
.stderr gets highlighted in the terminal, right? Maybe we output to stderr and then keep going.
Also if we use
node
instead, we avoid the problem entirely.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.
I will try the node to in another PR.