sdk [FIX] tool-install: Use restore action config options

Addresses https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2221478

If user has an invalid PackageSource on NuGet.config and attempts to install a tool with the --ignore-failed-sources attribute, it still outputs an error.

RestoreActionConfig needs to be passed down to NuGetPackageDownloader

Oct 18 '24 18:10 edvilme

Should also address https://github.com/dotnet/sdk/issues/36473

Oct 18 '24 20:10 edvilme

Missed adding the RestoreActionConfig to some other methods. It should be working fine now

Oct 22 '24 20:10 edvilme

@baronfel Do we want to backport this to 8.0 once its good? I believe we should. Probably 8.0.3xx?

Oct 22 '24 23:10 nagilson

When was the regression introduced?

Oct 22 '24 23:10 baronfel

When was the regression introduced?

Sometime between .NET 8 RC2 and .NET 7, most likely rc2

Oct 22 '24 23:10 nagilson

Ok then I'd say we should backport all the way back to 8.0.1xx so that source build gets it too.

Oct 22 '24 23:10 baronfel

Fantastic job figuring the correct changes out and also investigating the tests :) I have a remaining comment I'd like to hear on before we can merge this. There's also that final test you're working on. But the change itself looks great!

Thanks a lot! And thanks a lot for all the support. I also had the concern on the internal vs private, I am happy to look into how the mock pattern as to not expose this unnecessarily. Let me know :)

I have also added the tests to check if the flag works correctly (aka, it does not throw when the feeds are invalid and the --ignore-failed-sources flag is used

Oct 22 '24 23:10 edvilme

Maybe we can use https://api.nuget.org/v3/invalid.json ? That way we at least have some more influence/knowledge on what goes on with the urlSent from my iPhoneOn Oct 22, 2024, at 4:49 PM, Noah Gilson @.***> wrote: @nagilson commented on this pull request.

In test/dotnet.Tests/CommandTests/ToolInstallGlobalOrToolPathCommandTests.cs:

@@ -975,6 +989,16 @@ public void SetPermission(string path, string chmodArgument) { } }

   private string _nugetConfigWithInvalidSources = @"{

+ +

<add key=""nuget"" value=""https://api.nuget.org/v3/index.json"" />
<add key=""invalid_source"" value=""https://invalidsource.com/v3/index.json"" />

@dsplaisted Is there a standard feed we can use for testing an invalid source? I couldnt find one. Im not sure I like relying on this website but I dont have anything better unless we have something internal we can use for this.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: @.***>

Oct 23 '24 05:10 edvilme

Maybe we can use https://api.nuget.org/v3/invalid.json ? That way we at least have some more influence/knowledge on what goes on with the url

Yes, that seems like a good idea so that it's a domain we control.

Oct 23 '24 11:10 dsplaisted

All of my comments are resolved now, so this should be good to go. while the blazor wasm test failure is not related to your change, it does look like one remaining test is failing.

I would suggest you prioritize this issue above all else because it seems to impact the most customers.

After that, I would backport your changes to release/8.0.1xx. Don't merge that PR until you get tactics approval and then approval after that to confirm the branch and release is open for changes. You can use the bot to create backports: #43607 (comment) Like how I did here. In that linked backport PR is a template to get tactics approval. Youll need to fill that out and do manual testing to verify that the fix worked on 8.0.1xx as well. After that, youll need to send an email to the .NET Tactics alias with the same information asking for approval and add the label 'servicing-consider.'

By the way, why are we doing all of this... this was broken starting in 8.0 and most people are going to be on 8.0 since its the latest stable and long term support version of .NET. We want them to get this fix because it's important. But because its a stable and already released build, the quality / QA check bar is a lot higher, so we have a process in place for these changes

Oct 23 '24 23:10 nagilson

it does look like one remaining test is failing

Hello. Yes, I pushed a fix recently and it should be passing now on the ci/cd. I am rebasing and squashing the commits to avoid having lots of little commits.

Oct 24 '24 18:10 edvilme

You can squah and merge the PR without having to rebase or do any extra work by clicking on this button 😉

@/rainersigwald is very good about having a clean PR history and always squashing but our team is much less loose on that sort of thing. I would generally recommend to squash and merge the PR using the GitHub option.

Force-pushing makes it harder to review the PR, because all of the existing changes get wiped away and the order of commits are lost, so it makes it so the entire PR needs to be reviewed again. This also saves you from having to wait for the pipeline to run again, and also if there is some other test that keeps failing that's not your fault, you dont have to deal with it a second/third/etc time~

Oct 24 '24 18:10 nagilson

/backport to to release/8.0.1xx

Oct 24 '24 18:10 edvilme

Started backporting to to: https://github.com/dotnet/sdk/actions/runs/11505684487

Oct 24 '24 18:10 github-actions[bot]

@edvilme an error occurred while backporting to to, please check the run log for details!

Error: @edvilme is not a repo collaborator, backporting is not allowed. If you're a collaborator please make sure your dotnet team membership visibility is set to Public on https://github.com/orgs/dotnet/people?query=edvilme

Oct 24 '24 18:10 github-actions[bot]

/backport to to release/8.0.1xx

Oct 24 '24 20:10 edvilme

Started backporting to to: https://github.com/dotnet/sdk/actions/runs/11506674169