Skip to content
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

Set session socket into environment variable #6282

Merged
merged 3 commits into from
Jun 22, 2023

Conversation

code-asher
Copy link
Member

@code-asher code-asher commented Jun 21, 2023

While I was at it I added a CLI flag to override the default. I also swapped the default to --user-data-dir.

The value is set on an environment variable so it can be used by the extension host similar to VSCODE_IPC_HOOK_CLI.

Continuation of #6278 for #6275. We changed the socket path but did not propagate that change down to VS Code yet. The environment variable takes care of that.

@code-asher code-asher requested a review from a team as a code owner June 21, 2023 22:31
@code-asher
Copy link
Member Author

Going to add an e2e test as well since I think we can do that fairly easily.

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #6282 (c56b194) into main (56d10d8) will increase coverage by 0.08%.
The diff coverage is 88.23%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6282      +/-   ##
==========================================
+ Coverage   73.59%   73.67%   +0.08%     
==========================================
  Files          31       31              
  Lines        1863     1869       +6     
  Branches      399      401       +2     
==========================================
+ Hits         1371     1377       +6     
  Misses        415      415              
  Partials       77       77              
Impacted Files Coverage Δ
src/node/entry.ts 0.00% <0.00%> (ø)
src/node/cli.ts 90.80% <92.30%> (+0.24%) ⬆️
src/node/app.ts 100.00% <100.00%> (ø)
src/node/vscodeSocket.ts 87.37% <100.00%> (-0.13%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56d10d8...c56b194. Read the comment docs.

@code-asher code-asher force-pushed the configure-session-socket branch 6 times, most recently from 9884812 to 38107f3 Compare June 22, 2023 02:13
These flags mean the user explicitly wants to open in an existing
instance so if the socket is down it should error rather than try to
spawn code-server normally.
While I was at it I added a CLI flag to override the default.  I also
swapped the default to --user-data-dir.

The value is set on an environment variable so it can be used by the
extension host similar to VSCODE_IPC_HOOK_CLI.
@benz0li
Copy link
Contributor

benz0li commented Jun 22, 2023

@code-asher FYI Release v4.14.1-rc.1 (which does not include this change) work just fine.

@code-asher
Copy link
Member Author

code-asher commented Jun 22, 2023

Thank you! I will make one more RC so I can make a new release with this bugfix.

@code-asher
Copy link
Member Author

code-asher commented Jun 22, 2023

@sleexyz just a ping in case you wanted to check this over. Apologies for unilaterally continuing the fix; I was a bit anxious to finish it because I had just released the RC with our partial fix and I figured we should add an e2e test and did not want to saddle you with that (the e2e scaffolding can be difficult to work with). Please let me know if anything looks dumb.

Going to merge for now so I can start the next RC tonight but if something needs to be fixed we can always do another RC.

@code-asher code-asher merged commit 5c19962 into coder:main Jun 22, 2023
10 checks passed
@code-asher code-asher deleted the configure-session-socket branch June 22, 2023 06:47
@sleexyz
Copy link
Contributor

sleexyz commented Jun 22, 2023

Looks great! Thanks for the heads up, and sorry for the oversight 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants