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

fix(socket): use xdgBasedir.runtime instead of tmp #3304

Merged
merged 5 commits into from
May 13, 2021
Merged

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented May 6, 2021

This PR fixes this issue:

discarding socket connection: EACCES: permission denied, unlink '/tmp/code-server/tls-proxy'

by using the xdgBasedir.runtime path instead of /tmp/code-server/tls-proxy.

Changes

  • update pr template to say default branch
  • add runtime to getEnvPaths
  • add test for getEnvPaths
  • remove use of tmpdir in socket.ts

refactor: use paths.runtime in socket proxyPipe

Screenshots

image

Checklist

  • tested locally
  • added a test
  • updated CHANGELOG

Fixes #2200

@jsjoeio jsjoeio self-assigned this May 6, 2021
@jsjoeio jsjoeio added this to the v3.9.4 milestone May 6, 2021
@codecov
Copy link

codecov bot commented May 6, 2021

Codecov Report

Merging #3304 (2accb1a) into main (8e21eb5) will increase coverage by 0.26%.
The diff coverage is 100.00%.

❗ Current head 2accb1a differs from pull request most recent head 46fe77d. Consider uploading reports for the commit 46fe77d to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3304      +/-   ##
==========================================
+ Coverage   58.95%   59.21%   +0.26%     
==========================================
  Files          35       35              
  Lines        1703     1709       +6     
  Branches      374      379       +5     
==========================================
+ Hits         1004     1012       +8     
+ Misses        561      559       -2     
  Partials      138      138              
Impacted Files Coverage Δ
src/node/socket.ts 89.83% <100.00%> (-0.17%) ⬇️
src/node/util.ts 55.23% <100.00%> (+5.23%) ⬆️

Continue to review full report at Codecov.

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

@jsjoeio jsjoeio modified the milestones: v3.10.0, v3.11.0 May 7, 2021
@jsjoeio jsjoeio added the enhancement Some improvement that isn't a feature label May 10, 2021
@jsjoeio jsjoeio force-pushed the jsjoeio/fix-tmp-path branch 3 times, most recently from f883ab0 to acfd152 Compare May 10, 2021 23:32
@jsjoeio jsjoeio marked this pull request as ready for review May 10, 2021 23:32
@jsjoeio jsjoeio requested a review from a team as a code owner May 10, 2021 23:32
@jsjoeio jsjoeio requested a review from code-asher May 10, 2021 23:32
src/node/socket.ts Outdated Show resolved Hide resolved
src/node/util.ts Outdated Show resolved Hide resolved
src/node/util.ts Outdated Show resolved Hide resolved
test/unit/node/util.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@jawnsy jawnsy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 🕺

src/node/util.ts Show resolved Hide resolved
test/unit/node/util.test.ts Outdated Show resolved Hide resolved
@jsjoeio jsjoeio force-pushed the jsjoeio/fix-tmp-path branch 2 times, most recently from 3365791 to 4ec028a Compare May 10, 2021 23:47
CHANGELOG.md Outdated Show resolved Hide resolved
src/node/socket.ts Outdated Show resolved Hide resolved
src/node/socket.ts Outdated Show resolved Hide resolved
src/node/util.ts Outdated Show resolved Hide resolved
src/node/util.ts Outdated Show resolved Hide resolved
test/unit/node/util.test.ts Outdated Show resolved Hide resolved
@jsjoeio jsjoeio force-pushed the jsjoeio/fix-tmp-path branch 2 times, most recently from dfe8b7d to dd8f709 Compare May 11, 2021 21:37
@jsjoeio jsjoeio marked this pull request as draft May 12, 2021 18:00
@jsjoeio
Copy link
Contributor Author

jsjoeio commented May 12, 2021

Moving to draft because I need to write more tests (new change decreases -0.15%). We want increases!

@jsjoeio jsjoeio marked this pull request as ready for review May 12, 2021 20:58
@jsjoeio jsjoeio requested a review from code-asher May 12, 2021 20:58
Comment on lines +16 to +19
return () => ({
data: "/home/envPath/.local/share",
config: "/home/envPath/.config",
temp: "/tmp/envPath/runtime",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For whatever reason, when I extract this object into a variable and pass in to the mock, it doesn't work 🤷‍♂️

Comment on lines +33 to +38
jest.mock("xdg-basedir", () => ({
data: "/home/usr/.local/share",
config: "/home/usr/.config",
runtime: "/tmp/runtime",
}))
const getEnvPaths = require("../../../src/node/util").getEnvPaths
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to do this because of: jestjs/jest#2582 (comment)

@jsjoeio jsjoeio force-pushed the jsjoeio/fix-tmp-path branch 2 times, most recently from faf12cb to 4da1388 Compare May 12, 2021 21:51
@jsjoeio jsjoeio enabled auto-merge May 13, 2021 19:10
@jsjoeio jsjoeio merged commit d2337bc into main May 13, 2021
@jsjoeio jsjoeio deleted the jsjoeio/fix-tmp-path branch May 13, 2021 19:14
@coder coder deleted a comment May 14, 2021
@coder coder deleted a comment May 14, 2021
@coder coder deleted a comment May 14, 2021
@coder coder deleted a comment May 14, 2021
@coder coder deleted a comment May 14, 2021
@coder coder deleted a comment May 14, 2021
@coder coder deleted a comment May 14, 2021
Copy link

@Atom14011985 Atom14011985 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Some improvement that isn't a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fail to create tls-proxy for the second process.
4 participants