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

adding Bits-Transfer & user switch to install latest user profile insiders edition #1477

Merged
merged 8 commits into from
Aug 23, 2018

Conversation

tabs-not-spaces
Copy link
Contributor

@tabs-not-spaces tabs-not-spaces commented Aug 14, 2018

PR Summary

Quick update,

  • Moved the download line from Invoke-WebRequest to Start-BitsTransfer (better end user experience).
  • Added a new $BuildEdition value - Insider-User that allows the User Profile Insiders Edition to be installed.
  • Renamed current Insiders Edition value in $BuildEdition to Insider-System to reflect the two options.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets.
Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
  • Summarized changes
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

@rjmholt
Copy link
Contributor

rjmholt commented Aug 14, 2018

Hi @tabs-not-spaces, has the Install-VSCode.ps1 script been failing?

One consideration is that Start-BitsTransfer won't work on macOS or Linux.

But if download robustness is an issue, we could include a special case in the script for Windows. I'm told that for resilience scenarios, Start-BitsTransfer should be called with -Asynchronous too.

@tabs-not-spaces
Copy link
Contributor Author

@rjmholt currently the script only works for windows, but is set out in such a way to prepare for Linux / OSX in a future state.

I lean towards using start-bitstransfer as it provides a download progress bar which is beneficial for anyone on a slower internet connection.

Good idea on the async - I'll add that in and handle waiting for the file to complete.

@rjmholt
Copy link
Contributor

rjmholt commented Aug 14, 2018

Yes reading the script it's pretty clear it's only for Windows at the moment, so we can just switch over to Start-BitsTransfer and then ideally can work later to bring proper macOS/Linux support

@@ -167,13 +175,21 @@ if (!($IsLinux -or $IsOSX)) {
break;
}
}
if (($User -eq $true) -and ($BuildEdition -eq "Insider")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to do just if ($User -and ...) here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed and about to check in

@@ -122,6 +126,10 @@ param(
[ValidateSet("stable","insider")]
[string]$BuildEdition = "stable",

[Parameter()]
[switch]
Copy link
Contributor

Choose a reason for hiding this comment

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

The convention in the rest of the file seems to be putting the type constraint on the same line as the parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed and about to check in

@@ -56,6 +56,10 @@
A validated string defining which build edition or "stream" to download - stable or
insiders edition. If the parameter is not used, then stable is downloaded as default.

.PARAMETER User
Copy link
Contributor

Choose a reason for hiding this comment

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

The main function of this flag is to install VSCode-Insiders (user edition) right? Instead of non-Insiders edition. If that's the case, I would vote for the flag being called something like Insiders for now, and then if we want to make it more complex (e.g. choose between User Profile and non-User Profile) later, we could add another flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rjmholt I agree with you. In that case would it make sense for now then to move from the switch into the $BuildEdition parameter as a validation value?

Something like: [ValidateSet("stable","insider-system","insider-userprofile")] ??

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's a really good idea!

@rjmholt
Copy link
Contributor

rjmholt commented Aug 14, 2018

@tabs-not-spaces, this is a really great addition -- thanks for contributing!

@tabs-not-spaces tabs-not-spaces changed the title adding Bits-Transfer & user switch to install latest user profile insiders edition WIP: adding Bits-Transfer & user switch to install latest user profile insiders edition Aug 15, 2018
@tabs-not-spaces tabs-not-spaces changed the title WIP: adding Bits-Transfer & user switch to install latest user profile insiders edition adding Bits-Transfer & user switch to install latest user profile insiders edition Aug 15, 2018
@rjmholt rjmholt requested a review from TylerLeonhardt August 15, 2018 02:45
Copy link
Contributor

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

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

Ok, this looks good to me! Thanks again @tabs-not-spaces!

@tabs-not-spaces
Copy link
Contributor Author

@rjmholt found a major flaw with my code relating to the async behavior of bits transfer. have resolved and validated it now works!

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM! If you could just fix the merge conflict, we can put this in and republish the script :)

@rjmholt rjmholt merged commit 969e460 into PowerShell:master Aug 23, 2018
@rjmholt
Copy link
Contributor

rjmholt commented Aug 23, 2018

Thanks for your contribution @tabs-not-spaces!

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