Skip to content

Add VSCODE_PROXY_URI env var to extension host and terminal #3621

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

Closed
wants to merge 4 commits into from

Conversation

code-asher
Copy link
Member

@code-asher code-asher commented Jun 16, 2021

Checklist

  • Update CHANGELOG.md
  • Get proxy path from the client instead of req.headers.host.
  • Expose VSCODE_PROXY_URI to plugins
  • Expose VSCODE_PROXY_URI to terminal

@code-asher code-asher force-pushed the http-referer branch 3 times, most recently from 50d677b to 9cc76cc Compare June 16, 2021 22:21
@codecov
Copy link

codecov bot commented Jun 16, 2021

Codecov Report

Merging #3621 (fea63fe) into main (92d0d28) will increase coverage by 0.44%.
The diff coverage is 90.47%.

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

@@            Coverage Diff             @@
##             main    #3621      +/-   ##
==========================================
+ Coverage   64.94%   65.39%   +0.44%     
==========================================
  Files          36       36              
  Lines        1880     1887       +7     
  Branches      381      381              
==========================================
+ Hits         1221     1234      +13     
+ Misses        560      555       -5     
+ Partials       99       98       -1     
Impacted Files Coverage Δ
src/node/cli.ts 81.48% <71.42%> (+1.48%) ⬆️
src/common/util.ts 100.00% <100.00%> (ø)
src/node/util.ts 83.24% <0.00%> (+1.52%) ⬆️

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 92d0d28...ecb29c4. Read the comment docs.

@code-asher code-asher changed the title Http referer Add VSCODE_PROXY_URI env var to extension host and terminal Jun 21, 2021
@code-asher code-asher force-pushed the http-referer branch 3 times, most recently from 6d91937 to c0f7b0e Compare June 21, 2021 20:10
@code-asher
Copy link
Member Author

Currently on hold since I will need to rebase this on 1.57.

@code-asher code-asher added the blocked This issue cannot proceed due to external factors label Jun 22, 2021
@jsjoeio
Copy link
Contributor

jsjoeio commented Aug 5, 2021

@code-asher remind me and @jawnsy that we should write a test for the webview fix we did here: #3895 after this gets merged

@jsjoeio
Copy link
Contributor

jsjoeio commented Aug 24, 2021

bump @code-asher - anything I can do to help get these unblocked and ready to merge?

@code-asher
Copy link
Member Author

code-asher commented Aug 25, 2021 via email

@code-asher code-asher removed the blocked This issue cannot proceed due to external factors label Aug 25, 2021
@jsjoeio
Copy link
Contributor

jsjoeio commented Sep 13, 2021

@code-asher and I spoke offline about this. If we can get this in this week before Wednesday, great! But most likely shooting for the next release.

@jsjoeio
Copy link
Contributor

jsjoeio commented Sep 20, 2021

bump @code-asher -> think we'll be able to get this PR rebased, updated and marked ready this week?

@code-asher
Copy link
Member Author

code-asher commented Sep 20, 2021 via email

@code-asher code-asher force-pushed the http-referer branch 3 times, most recently from 7f8f2e6 to e91e201 Compare September 27, 2021 20:06
This will let us test extension-related features (like the proxy URI).

I removed the environment variables in the script because they override
the ones you set yourself. We still set defaults in constants.ts.
This way it can be called multiple times without incurring extra
overhead or potentially resolving in a different way.

Also catch any errors when parsing query options just as we do with the
options in the markup and log errors with the latter as well.
This should not be merged as-is since it points to my fork.
@jsjoeio
Copy link
Contributor

jsjoeio commented Sep 29, 2021

Man, we really need to fix #4027.

This is failing because your PR is from a fork (yet you're a maintainer and have access to m) 😢

image

@code-asher
Copy link
Member Author

code-asher commented Sep 29, 2021

All good to go, blocked by coder/vscode#3

@jsjoeio
Copy link
Contributor

jsjoeio commented Oct 29, 2021

Besides the conflicting files, is there anything blocking this?

(Happy to pick it up as well)

@code-asher
Copy link
Member Author

code-asher commented Oct 29, 2021 via email

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 2, 2021

Let me know if you need help moving this forward!

@jsjoeio jsjoeio requested review from GirlBossRush and removed request for GirlBossRush November 29, 2021 20:42
@jsjoeio
Copy link
Contributor

jsjoeio commented Dec 8, 2021

Too many changes and branch out-of-date. We'll see if we can get this ready for the next release.

@jsjoeio
Copy link
Contributor

jsjoeio commented Dec 14, 2021

@code-asher what do you think about this PR? Is it at the end of it's life?

@code-asher
Copy link
Member Author

code-asher commented Dec 15, 2021 via email

@code-asher
Copy link
Member Author

I will re-open this when it is ready to go.

@code-asher code-asher closed this Dec 17, 2021
@jsjoeio jsjoeio removed this from the 4.1.0 - improve iPad UX milestone Jan 7, 2022
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.

2 participants