3776 - Latest version reports it is old

[VineethReyya]

Issue describing the changes in this PR

Fix1 : when we install Azure Function Core Tool via chocolatey, the shim is generating twice as we have 3.exe files (in root folder inproc6 and inproc8 folder). Now we have added additional checks to ignore func.exe from subfolders (inproc6 and inproc8 folder) and generate shim for func.exe in root directory.

Fix 2 : In VersionHelper.cs we have changed the logic to check whether latestCoreToolsAssemblyZipFile( by extracting the segment contains '.zip' from the download link) contains CliVersion (by adding '.zip')

resolves #3776

Pull request checklist

• My changes do not require documentation changes
  • Otherwise: Documentation issue linked to PR
• My changes do not need to be backported to a previous version
  • Otherwise: Backport tracked by issue/PR #issue_or_pr
• I have added all required tests (Unit tests, E2E tests)

[liliankasem]

I'm concerned about creating a new instance of HttpClient here every time this method is called and not disposing it. I imagine you added the parameter here so you can provide a mocked client?

Instead, can we switch to either DI or a singleton for the HttpClient. With the singleton, you can still use a mocked client i.e.

// VersionHelper.cs
private static readonly HttpClient _httpClient = new HttpClient
{
    Timeout = TimeSpan.FromSeconds(1)
 };

// Test
// Set the singleton HttpClient to the mocked one for the test (using reflection or static setter)
        typeof(VersionHelper)
            .GetField("_httpClient", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static)
            .SetValue(null, httpClient);

DI is usually best

[VineethReyya]

Thanks for the review @liliankasem, I imagine you added the parameter here so you can provide a mocked client? -- Yes

we can dispose the client with using statement. Am I in the right direction?

using (client ??= new System.Net.Http.HttpClient{Timeout = TimeSpan.FromSeconds(1)})

please review the changes when you have a moment

[VineethReyya]

If we don't want to dispose the client then i can go with below code and its working fine
//versionhelper.cs
private static HttpClient _httpClient = new HttpClient { Timeout = TimeSpan.FromSeconds(1) };

//versiontest.cs

// Return HttpClient with mocked handler
var httpClient = new HttpClient(mockHandler.Object) { Timeout = TimeSpan.FromSeconds(1) };

typeof(VersionHelper)
.GetField("_httpClient", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static)
.SetValue(null, httpClient);

return httpClient;

[liliankasem]

Given this is only runs when the --help action is called, using a using statement here to dispose is fine. I didn't suggest this originally because I didn't think the using statement would work with the ??= operator, but if it does, let's do that.

[liliankasem]

@VineethReyya can you break out these changes into a different PR?

I think the chocolatey script change that you describe in the PR description is not related to the VersionHelper change. Given this PR handles two different fixes, I would like them to be their own PRs so it's easier to track changes that break anything, and also easier to revert a single fix instead of both at the same time.

[VineethReyya]

@VineethReyya can you break out these changes into a different PR?

I think the chocolatey script change that you describe in the PR description is not related to the VersionHelper change. Given this PR handles two different fixes, I would like them to be their own PRs so it's easier to track changes that break anything, and also easier to revert a single fix instead of both at the same time.

@liliankasem Both the fixes are related to same ticket.

[VineethReyya]

@liliankasem created separate PRs for different issues. Part I, Part II. please have a look and review the changes.

[VineethReyya]

closed this PR as new separate PRs created for fixes