C#: Check fallback nuget feeds before trying to use them in the fallb… #16164
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| catch (Exception exc) | ||
| { | ||
| logger.LogWarning($"Failed to get directory of '{config}': {exc}"); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause
| @@ -32,6 +33,12 @@ | |||
| return Success; | |||
| } | |||
|
|
|||
| public bool RunCommand(string args, string? workingDirectory, out IList<string> output) | |||
Check notice
Code scanning / CodeQL
Local scope variable shadows member
|
Ahh, the test failure is due to the way we test for timeouts: |
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.Nuget.cs
Show resolved
Hide resolved
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.Nuget.cs
Show resolved
Hide resolved
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.Nuget.cs
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private HashSet<string> GetAllFeeds(List<FileInfo> allFiles) | ||
| private (HashSet<string>, HashSet<string>) GetAllFeeds(List<FileInfo> allFiles) |
There was a problem hiding this comment.
Even though this method is private, maybe add some description of the return since it is a tuple of hashsets of the same type (to avoid confusing them)
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.Nuget.cs
Outdated
Show resolved
Hide resolved
csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/DependencyManager.Nuget.cs
Outdated
Show resolved
Hide resolved
| return feeds; | ||
|
|
||
| // todo: this could be improved. | ||
| // We don't have to get the feeds from each of the folders from below, it would be enought to check the folders that recursively contain the others. |
There was a problem hiding this comment.
So you are thinking of removing all folder paths that are prefixes of other path before calling the CLI?
There was a problem hiding this comment.
Yes, that would be an option. (But I won't do it for the time being, as it would add extra complexity)
tamasvajk
force-pushed
the
buildless/nuget-feed-fallback-feed-check
branch
from
1fdabcb to
c004f92
Compare
|
@michaelnebel I've applied your suggestions and rebased the PR. |
| logger.LogInfo($"Getting Nuget feeds from '{nugetConfig}'..."); | ||
| return dotnet.GetNugetFeeds(nugetConfig); | ||
| } | ||
| IList<string> GetNugetFeeds(string nugetConfig) => dotnet.GetNugetFeeds(nugetConfig); |
There was a problem hiding this comment.
You can just call dotnet.GetNugetFeeds directly instead of declaring a local function (same comment below).
|
I just saw the comment https://github.com/github/codeql-csharp-team/issues/432#issuecomment-2051098433 |
There are definitely ways to improve this. Let's get some feedback from the field team before tackling it. |
…ack restore process
This PR is improving the fallback nuget restore process by checking if the used nuget feeds are reachable or not. Currently the fallback restore process doesn't use a
nuget.configfile. This PR is changing this. The generatednuget.configuses the reachable fallback feeds in the<packageSources>. Additionally, we're logging the nuget feeds that are inherited from the user levelnuget.config, but we're not (yet) checking their reachability.