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

PowerShellForGitHub: Disable Progress Bar for Invoke-WebRequest #229

Merged
merged 6 commits into from
Jun 10, 2020
Merged

PowerShellForGitHub: Disable Progress Bar for Invoke-WebRequest #229

merged 6 commits into from
Jun 10, 2020

Conversation

X-Guardian
Copy link
Contributor

@X-Guardian X-Guardian commented Jun 8, 2020

Description

This PR disables the PowerShell progress bar for the Invoke-WebRequest cmdlet calls in the Invoke-GHRestMethod and Invoke-SendTelemetryEvent functions due to known performance issues in PowerShell 5.1.

Issues Fixed

References

Progress bar can significantly impact cmdlet performance.

Checklist

  • You actually ran the code that you just wrote, especially if you did just "one last quick change".
  • Comment-based help added/updated, including examples.
  • Static analysis is reporting back clean.
  • New/changed code adheres to our coding guidelines.
  • Changes to the manifest file follow the manifest guidance.
  • Unit tests were added/updated and are all passing. See testing guidelines.
  • Relevant usage examples have been added/updated in USAGE.md.
  • If desired, ensure your name is added to our Contributors list

@X-Guardian X-Guardian marked this pull request as ready for review June 8, 2020 19:11
Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix. I think we need to consider the failure scenario though to make sure that we're not accidentally making a permanent change to the user's preferences.

GitHubCore.ps1 Outdated Show resolved Hide resolved
GitHubCore.ps1 Outdated Show resolved Hide resolved
@X-Guardian
Copy link
Contributor Author

I was still getting a progress bar 'flicker' with NoStatus set, and I tracked it down to the Invoke-WebRequest calls in the Telemetry module, so I have added the same code to disable the progress bar to that module too.

@HowardWolosky
Copy link
Member

I was still getting a progress bar 'flicker' with NoStatus set, and I tracked it down to the Invoke-WebRequest calls in the Telemetry module, so I have added the same code to disable the progress bar to that module too.

I had been seeing that too and was wondering where it was coming from. Glad you tracked it down.

It is a shame that there's duplicated code across Invoke-GHRestMethod and Invoke-SendTelemetryEvent, but refactoring between those two was difficult, and in the end I wanted to see if I could keep that file as isolated as possible so that it could be more easily re-purposed by others in other projects. I'm intending to write something up on that (and possibly split it off into another project) at some point.

Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

Thanks for this. Will get it merged in later today. CI queue is a bit backed-up at the moment.

@X-Guardian
Copy link
Contributor Author

X-Guardian commented Jun 9, 2020

Could you not create two more GitHub accounts/organizations, to have one for each test environment (Windows, Linux, MacOS)? The pipeline jobs could run in parallel then, with no impact on each other.

It would reduce the CI runtime from an hour to approx 20 mins.

@HowardWolosky
Copy link
Member

Could you not create two more GitHub accounts/organizations, to have one for each test environment (Windows, Linux, MacOS)? The pipeline jobs could run in parallel then, with no impact on each other.

It would reduce the CI runtime from an hour to approx 20 mins.

Let's see how well this works.... #231.

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky HowardWolosky merged commit 6e794cb into microsoft:master Jun 10, 2020
@X-Guardian X-Guardian deleted the Disable-InvokeWebRequest-ProgressBar branch June 10, 2020 20:02
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.

PowerShellForGitHub: Suggest Disabling ProgressBar when Calling Invoke-WebRequest
2 participants