Adds the ability to use a proxy, Closes #2698 #4679

nicodecleyre · 2023-03-23T21:42:03Z

Closes #2698

milanholemans · 2023-03-24T08:50:46Z

Nice work, we'll review it ASAP!

src/settingsNames.ts

src/request.ts

waldekmastykarz

Almost there! Let's do a couple more adjustments and clarify the use of ports vs. protocols.

docs/docs/user-guide/using-proxy-url.md

src/request.ts

waldekmastykarz

Almost there! Let's do a couple more adjustments and clarify the use of ports vs. protocols.

nicodecleyre · 2023-05-13T18:56:30Z

Thank you for the review @waldekmastykarz, the PR is updated! Can you take another look?

waldekmastykarz · 2023-05-15T08:50:04Z

Will do 👍

docs/docs/user-guide/using-proxy-url.md

waldekmastykarz

Hey @nicodecleyre, sorry for late reply. Following our discussion and like suggested here: #2698 (comment), let's align with the common convention for Node.js apps and use the standard environment variables HTTP_PROXY and HTTPS_PROXY instead of introducing our own setting. That will make it more consistent with other Node.js apps and make the CLI more intuitive to use for folks who use Node.js.

nicodecleyre · 2023-07-17T21:41:55Z

Hey @nicodecleyre, sorry for late reply. Following our discussion and like suggested here: #2698 (comment), let's align with the common convention for Node.js apps and use the standard environment variables HTTP_PROXY and HTTPS_PROXY instead of introducing our own setting. That will make it more consistent with other Node.js apps and make the CLI more intuitive to use for folks who use Node.js.

Hey @waldekmastykarz, would be a nice change if we could use the environment variables. A couple of months ago I did some research on this but wasn't able to find how to get this working in all interfaces/environments. Do you know by any chance if there is a call for these environment variables across all interfaces/environments?

waldekmastykarz · 2023-08-03T18:22:12Z

Not sure what you mean with "across all interfaces/environments". Could you please elaborate @nicodecleyre? Also, looking at the PR once again, I realized that we haven't added support for proxying application insights calls, which has its own proxy setup. We've changed how telemetry is run #5327 but haven't merged it yet, so let's so let's ensure that we include the necessary changes there as well when we process this PR.

nicodecleyre · 2023-09-01T09:14:55Z

Not sure what you mean with "across all interfaces/environments". Could you please elaborate @nicodecleyre? Also, looking at the PR once again, I realized that we haven't added support for proxying application insights calls, which has its own proxy setup. We've changed how telemetry is run #5327 but haven't merged it yet, so let's so let's ensure that we include the necessary changes there as well when we process this PR.

Hi @waldekmastykarz, what I mean with "across all interfaces/environments" is how do we ensure that we can retrieve the value of these environment variables in:

cmd in Windows
PowerShell in Windows
terminal in mac

as per my knowledge, you can set a env var in cmd like this:

but when i try to get the same env var in PS, it's not present:

No offense intended, but you said here that setting environment variable differs per shell and you don't think we can do anything about it. So i'm having a bit of a feeling that we're having the same discussion again 😄

I've looked at the PR, but I don't quite understand what you mean with "support for proxying application insights calls", can you elaborate what you expect for this in this PR?

waldekmastykarz · 2023-09-03T10:34:19Z

Check, so with "environments" you mean "shells", understood.

Hi @waldekmastykarz, what I mean with "across all interfaces/environments" is how do we ensure that we can retrieve the value of these environment variables in:

While setting environment variables differs per shell like we discussed, it shouldn't be relevant to us because we only care about reading their value, which should be standardized and available through process.env.YOUR_ENV_VAR in Node.js. So while each shell will come with its own way to set these variables, that goes outside of our scopes, because we care only about the fact whether a variable is set or not, and not how it's been set.

I've looked at the PR, but I don't quite understand what you mean with "support for proxying application insights calls", can you elaborate what you expect for this in this PR?

In CLI for Microsoft 365 we use Application Insights to track telemetry. Application Insights comes with its own instrumentation for proxy and this PR should pass the proxy configuration to Application Insights too. In its current state, if the user set a proxy on their machine, our telemetry collection would not work.

nicodecleyre · 2023-09-24T20:02:01Z

Hi @waldekmastykarz, I've included the telemetry changes from the PR you provided. But it keeps failing on 3 tests, can i get some guidance on this please?

waldekmastykarz · 2023-09-27T18:09:30Z

Sure thing! Let me have a look

src/appInsights.ts

src/telemetry.ts

waldekmastykarz · 2023-09-27T18:12:43Z

Hey @nicodecleyre, is your branch still up to date? I'm seeing some odd changes as a part of the PR that don't seem to be related and seem like something that might've been pulled in? Could you please double check before we continue the review? Thanks!

nicodecleyre · 2023-09-27T18:32:21Z

Hey @nicodecleyre, is your branch still up to date? I'm seeing some odd changes as a part of the PR that don't seem to be related and seem like something that might've been pulled in? Could you please double check before we continue the review? Thanks!

Hi @waldekmastykarz, I took the telemetry changes from the pr #5327 which isn't merged yet, as you asked. But maybe I misunderstood what you were trying to say?

waldekmastykarz · 2023-09-30T11:47:23Z

Ah sorry, right. That got me confused. I thought we've already merged #5327 which isn't the case. All good.

waldekmastykarz · 2023-10-22T13:21:39Z

Hey @nicodecleyre, now that we've merged #5327, could you please do one final rebase so that we can review your proposed changes and get them in? Thanks again for your patience 😊

waldekmastykarz

Hey @nicodecleyre, when I rebase your PR onto the latest main, I'm getting 2 test failing:

Could you please have a look at what's wrong? I confirmed that these tests only fail after rebasing your PR. If I get latest from main, all tests pass and I get 100% coverage.

nicodecleyre · 2023-12-27T15:18:08Z

Hi @waldekmastykarz, rebase done ✅

waldekmastykarz

Almost there. Let's do a few more adjustments before we merge it

src/request.spec.ts

src/request.ts

waldekmastykarz

Very nicely done with a few cosmetic changes I've added when merging the PR 👏

waldekmastykarz · 2024-01-06T11:16:36Z

src/request.spec.ts

@@ -396,10 +396,14 @@ describe('Request', () => {
  });

  it('returns response of a successful GET request, with a proxy url', (done) => {
+    let outcome = false;


It would be clearer to name the variable after what it represents, eg. proxyConfigured

waldekmastykarz · 2024-01-06T11:20:17Z

src/request.spec.ts

    sinon.stub(process, 'env').value({ 'HTTPS_PROXY': 'http://proxy.contoso.com:8080' });

    sinon.stub(_request as any, 'req').callsFake((options) => {
      _options = options as CliRequestOptions;
+      if (_options.proxy && _options.proxy.host === 'proxy.contoso.com' && _options.proxy.port === 8080 && _options.proxy.protocol === 'http') {


We can simplify this code by assigning the outcome of the comparison directly to the proxyConfigured variable without an if

waldekmastykarz · 2024-01-06T11:20:54Z

src/request.spec.ts

    sinon.stub(process, 'env').value({ 'HTTPS_PROXY': 'http://username:password@proxy.contoso.com:8080' });

    sinon.stub(_request as any, 'req').callsFake((options) => {
      _options = options as CliRequestOptions;
+      if (_options.proxy && _options.proxy.host === 'proxy.contoso.com'
+        && _options.proxy.port === 8080


Let's put operators at the end of line for readability

waldekmastykarz · 2024-01-06T11:26:25Z

src/settingsNames.ts

@@ -14,6 +14,7 @@ const settingsNames = {
  printErrorsAsPlainText: 'printErrorsAsPlainText',
  prompt: 'prompt',
  promptListPageSize: 'promptListPageSize',
+  proxyUrl: 'proxyUrl',


We should remove it since we're not using it

waldekmastykarz · 2024-01-06T13:09:04Z

Merged manually. Thank you! 👏

waldekmastykarz reviewed Mar 28, 2023

View reviewed changes

src/settingsNames.ts Outdated Show resolved Hide resolved

waldekmastykarz reviewed Mar 28, 2023

View reviewed changes

src/request.ts Outdated Show resolved Hide resolved

nicodecleyre force-pushed the m365-with-proxy branch from 9c7c16f to 76a3075 Compare May 10, 2023 14:29

waldekmastykarz requested changes May 13, 2023

View reviewed changes

waldekmastykarz marked this pull request as draft May 13, 2023 07:52

nicodecleyre marked this pull request as ready for review May 13, 2023 18:55

Jwaegebaert force-pushed the main branch from bb31388 to f0e1354 Compare May 31, 2023 08:48

Jwaegebaert reviewed May 31, 2023

View reviewed changes

docs/docs/user-guide/using-proxy-url.md Outdated Show resolved Hide resolved

Jwaegebaert marked this pull request as draft May 31, 2023 09:26

nicodecleyre force-pushed the m365-with-proxy branch from 8eeaa1c to 9ee9952 Compare June 5, 2023 19:35

nicodecleyre marked this pull request as ready for review June 5, 2023 19:35

Jwaegebaert assigned waldekmastykarz Jul 5, 2023

waldekmastykarz requested changes Jul 16, 2023

View reviewed changes

waldekmastykarz marked this pull request as draft August 13, 2023 07:54

nicodecleyre force-pushed the m365-with-proxy branch from 7d92a27 to 1eea5b2 Compare September 1, 2023 09:43

waldekmastykarz reviewed Sep 27, 2023

View reviewed changes

src/appInsights.ts Outdated Show resolved Hide resolved

src/appInsights.ts Outdated Show resolved Hide resolved

src/telemetry.ts Outdated Show resolved Hide resolved

waldekmastykarz requested changes Dec 24, 2023

View reviewed changes

waldekmastykarz marked this pull request as draft December 24, 2023 08:54

nicodecleyre and others added 16 commits December 27, 2023 16:10

Enhances m365 with ability to use proxy

8554d5b

temp

f0b6a14

Enhances m365 with proxy

cf3879a

Update settingsNames.ts

59209a6

Updates doc & post usage

7224197

wrong link

f403497

update docs

c5e8c82

url in doc

88d9148

fix

a4323aa

Fix import ES module

ebc524e

proxy environment vars

d064bfd

temp commit

7021f5b

added proxy telemetry

55e5797

rebase

2983845

Tested and working version

9c1cdfc

Typo

0140636

nicodecleyre force-pushed the m365-with-proxy branch from bf7410a to 0140636 Compare December 27, 2023 15:12

nicodecleyre marked this pull request as ready for review December 27, 2023 15:18

waldekmastykarz requested changes Jan 3, 2024

View reviewed changes

src/request.spec.ts Show resolved Hide resolved

src/request.ts Outdated Show resolved Hide resolved

src/request.ts Outdated Show resolved Hide resolved

waldekmastykarz marked this pull request as draft January 3, 2024 12:08

Changes

0dc65a7

nicodecleyre marked this pull request as ready for review January 3, 2024 17:18

waldekmastykarz approved these changes Jan 6, 2024

View reviewed changes

waldekmastykarz reviewed Jan 6, 2024

View reviewed changes

waldekmastykarz added the pr-merged label Jan 6, 2024

waldekmastykarz closed this Jan 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds the ability to use a proxy, Closes #2698 #4679

Adds the ability to use a proxy, Closes #2698 #4679

nicodecleyre commented Mar 23, 2023

milanholemans commented Mar 24, 2023

waldekmastykarz left a comment

waldekmastykarz left a comment

nicodecleyre commented May 13, 2023

waldekmastykarz commented May 15, 2023

waldekmastykarz left a comment

nicodecleyre commented Jul 17, 2023

waldekmastykarz commented Aug 3, 2023

nicodecleyre commented Sep 1, 2023

waldekmastykarz commented Sep 3, 2023

nicodecleyre commented Sep 24, 2023

waldekmastykarz commented Sep 27, 2023

waldekmastykarz commented Sep 27, 2023

nicodecleyre commented Sep 27, 2023

waldekmastykarz commented Sep 30, 2023

waldekmastykarz commented Oct 22, 2023

waldekmastykarz left a comment

nicodecleyre commented Dec 27, 2023

waldekmastykarz left a comment

waldekmastykarz left a comment

waldekmastykarz Jan 6, 2024

waldekmastykarz Jan 6, 2024

waldekmastykarz Jan 6, 2024

waldekmastykarz Jan 6, 2024

waldekmastykarz commented Jan 6, 2024

Adds the ability to use a proxy, Closes #2698 #4679

Adds the ability to use a proxy, Closes #2698 #4679

Conversation

nicodecleyre commented Mar 23, 2023

milanholemans commented Mar 24, 2023

waldekmastykarz left a comment

Choose a reason for hiding this comment

waldekmastykarz left a comment

Choose a reason for hiding this comment

nicodecleyre commented May 13, 2023

waldekmastykarz commented May 15, 2023

waldekmastykarz left a comment

Choose a reason for hiding this comment

nicodecleyre commented Jul 17, 2023

waldekmastykarz commented Aug 3, 2023

nicodecleyre commented Sep 1, 2023

waldekmastykarz commented Sep 3, 2023

nicodecleyre commented Sep 24, 2023

waldekmastykarz commented Sep 27, 2023

waldekmastykarz commented Sep 27, 2023

nicodecleyre commented Sep 27, 2023

waldekmastykarz commented Sep 30, 2023

waldekmastykarz commented Oct 22, 2023

waldekmastykarz left a comment

Choose a reason for hiding this comment

nicodecleyre commented Dec 27, 2023

waldekmastykarz left a comment

Choose a reason for hiding this comment

waldekmastykarz left a comment

Choose a reason for hiding this comment

waldekmastykarz Jan 6, 2024

Choose a reason for hiding this comment

waldekmastykarz Jan 6, 2024

Choose a reason for hiding this comment

waldekmastykarz Jan 6, 2024

Choose a reason for hiding this comment

waldekmastykarz Jan 6, 2024

Choose a reason for hiding this comment

waldekmastykarz commented Jan 6, 2024