-
Notifications
You must be signed in to change notification settings - Fork 58
feat(vscode-web): add offline, use_cached, extensions_dir and auto_install_extensions #235
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
feat(vscode-web): add offline, use_cached, extensions_dir and auto_install_extensions #235
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.
Looks good to me, but I think we should use the same name in all the comments/output/functions (maybe VS Code Web
).
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 I have suggested all the name changes, but I could have missed some.
Co-authored-by: Muhammad Atif Ali <me@matifali.dev>
Co-authored-by: Muhammad Atif Ali <me@matifali.dev>
Co-authored-by: Muhammad Atif Ali <me@matifali.dev>
Co-authored-by: Muhammad Atif Ali <me@matifali.dev>
Co-authored-by: Muhammad Atif Ali <me@matifali.dev>
Co-authored-by: Muhammad Atif Ali <me@matifali.dev>
Co-authored-by: Muhammad Atif Ali <me@matifali.dev>
Co-authored-by: Muhammad Atif Ali <me@matifali.dev>
Co-authored-by: Muhammad Atif Ali <me@matifali.dev>
Co-authored-by: Muhammad Atif Ali <me@matifali.dev>
Co-authored-by: Muhammad Atif Ali <me@matifali.dev>
some of this was there from before and some of it was exactly working code from the code-server implementation. I have accepted all your suggestions, but i will need to retest and fix CI errors. |
Thank you, @michaelbrewer ❤️ . You are doing a great job of making modules super helpful. Let's wait for an approval from @code-asher and I will merge and cut a release. |
Would be nice to have official templates is composed of modules. :) |
I have also thought about this and created an issue. But Coder bundles starter templates within the product and when we add modules that are fetched from registry, this does not work on restricted or air-gapped deployments. |
Most of the included examples don't run air-gapped as they would curl to install code-server. Maybe https://developer.hashicorp.com/terraform/language/providers/requirements#source-addresses to allow for |
Scanning through the examples you can see cases of this:
|
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.
Thank you @matifali for all finding all the spots to update the name! Absolutely perfect.
I made a few more minor suggestions that I missed the first time around, nothing worth blocking over though so feel free to merge.
Thank you @michaelbrewer for putting this together!
if [ "${AUTO_INSTALL_EXTENSIONS}" = true ]; then | ||
if ! command -v jq > /dev/null; then | ||
echo "jq is required to install extensions from a workspace file." | ||
exit 0 |
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?
exit 0 | |
exit 1 |
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.
Co-authored-by: Asher <ash@coder.com>
Co-authored-by: Asher <ash@coder.com>
Co-authored-by: Asher <ash@coder.com>
Co-authored-by: Asher <ash@coder.com>
.vscode/extensions.json
#231