-
Notifications
You must be signed in to change notification settings - Fork 103
Changes to understand productVersion.txt if found in container with a build #76
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
Changes to understand productVersion.txt if found in container with a build #76
Conversation
|
Relates to #65 |
|
@nohwnd Could you review this? |
… when installing runtimes.
339ed13 to
5aea6ec
Compare
src/dotnet-install.ps1
Outdated
| $ProductVersionTxtURL = "$AzureFeed/Sdk/$SpecificVersion/productVersion.txt" | ||
| } | ||
| else { | ||
| throw "Invalid value specified for `$Runtime" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the ` preceeding $Runtime there by design?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| throw "Invalid value specified for `$Runtime" | |
| throw "Invalid value '$Runtime' specified for `$Runtime." |
src/dotnet-install.ps1
Outdated
| $productVersion = $SpecificVersion | ||
| } | ||
| } catch { | ||
| Say-Verbose "Could not read productVersion.txt at $productVersionTxtUrl, so using default value of $SpecificVersion" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider including $_.Exception.Message in the string passed to Say-verbose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do; since the existing catches didn't I was trying to follow the pattern.
src/dotnet-install.ps1
Outdated
| $ProductVersionTxtURL = "$AzureFeed/Sdk/$SpecificVersion/productVersion.txt" | ||
| } | ||
| else { | ||
| throw "Invalid value specified for `$Runtime" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| throw "Invalid value specified for `$Runtime" | |
| throw "Invalid value '$Runtime' specified for `$Runtime." |
|
@donJoseLuis / @bozturkMSFT I think I've addressed the requests, could you look again? |
donJoseLuis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
difficult to sign off from a code review alone. Are there any tests? And if so, please call out if these didn't pass after these changes in your branch.
This repo doesn't seem to have tests, no... but the scripts (from before some PR feedback) are in use in https://github.com/dotnet/arcade/tree/master/eng/common/dotnet-install-scripts in most .NET Core repos today. |
This stems from the changes (which will be rolled back if and when this change becomes available in the "normal" scripts) done as part of dotnet/arcade#6051.
The changes in this PR are a copy of the scripts added in https://github.com/dotnet/arcade/tree/master/eng/common/dotnet-install-scripts . I've simply made it such that the scripts will look for the existence of productVersion.txt, and if found use this instead of the "specificVersion" variable to resolve the path to the package(s) being downloaded and unzipped.