Skip to content

Update the dotnet-install script URL in Fake.DotNet.getGenericDotNetC… #2853

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Numpsy
Copy link
Contributor

@Numpsy Numpsy commented Jan 12, 2025

…liInstallerUrl

refs #2848

Description

The scripts have been updated due to dotnet/core#9671, but according to https://github.com/dotnet/sdk/blob/main/scripts/obtain/dotnet-install-readme the source for the scripts was changed from https://github.com/dotnet/sdk to https://github.com/dotnet/install-scripts some years ago, so it looks like FAKE was already using an old version of the scripts?

@@ -136,7 +136,7 @@ module DotNet =
/// Get .NET Core SDK download uri
/// </summary>
let private getGenericDotNetCliInstallerUrl branch installerName =
sprintf "https://raw.githubusercontent.com/dotnet/cli/%s/scripts/obtain/%s" branch installerName
sprintf "https://raw.githubusercontent.com/dotnet/install-scripts/%s/src/%s" branch installerName
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really certain if this should be pulling the scripts from github or the 'distribution' urls such as https://dot.net/v1/dotnet-install.sh - but thats a bigger change which would mean dropping the configurable branch, so I just tried to keep it minimal for now

Copy link
Contributor

Choose a reason for hiding this comment

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

We will be sharing guidance shortly.

This issue should provide some insight: dotnet/install-scripts#558 (comment)

Copy link
Contributor

github-actions bot commented Jan 12, 2025

Test Results

   12 files  ±0     12 suites  ±0   55m 12s ⏱️ + 2m 30s
  451 tests ±0    450 ✅ ±0  1 💤 ±0  0 ❌ ±0 
1 287 runs  ±0  1 284 ✅ ±0  3 💤 ±0  0 ❌ ±0 

Results for commit 600973f. ± Comparison against base commit 73fc961.

♻️ This comment has been updated with latest results.

@Numpsy Numpsy marked this pull request as ready for review January 12, 2025 15:57
@xperiandri
Copy link
Collaborator

@Numpsy should I merge?

@Numpsy
Copy link
Contributor Author

Numpsy commented Mar 12, 2025

@Numpsy should I merge?

I don't think that the existing URL is right (the script there doesn't contain the distribution domain update), but it might need a call on if 'branch' is really useful or if it should always use https://dot.net/v1/ in future major versions

@richlander
Copy link
Contributor

We don't guarantee prod-level stability/response with the GitHub URL.

@Numpsy Numpsy force-pushed the install_script_url branch from 94cd1aa to 600973f Compare March 14, 2025 17:23
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