Skip to content

Analyzer to detect missing ASP.NET Core integration registration#1917

Merged
surgupta-msft merged 48 commits intomainfrom
surgupta/aspnet-analyzer
Oct 25, 2023
Merged

Analyzer to detect missing ASP.NET Core integration registration#1917
surgupta-msft merged 48 commits intomainfrom
surgupta/aspnet-analyzer

Conversation

@surgupta-msft
Copy link
Copy Markdown
Contributor

@surgupta-msft surgupta-msft commented Sep 20, 2023

Issue describing the changes in this PR

resolves #1902

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Added documentation in the PR
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • 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)

Additional information

Additional PR information

Comment thread docs/analyzer-rules/AZFW0014.md Outdated
Comment thread sdk/Sdk.Analyzers/RegistrationExpectedInASPNetIntegration.cs Outdated
Comment thread sdk/Sdk.Analyzers/RegistrationExpectedInASPNetIntegration.cs Outdated
Comment thread sdk/Sdk.Analyzers/RegistrationExpectedInASPNetIntegration.cs Outdated
Comment thread test/Sdk.Analyzers.Tests/RegistrationExpectedInAspNetIntegrationTests.cs Outdated
Comment thread sdk/Sdk.Analyzers/Extensions/MethodSymbolExtensions.cs Outdated
Comment thread sdk/Sdk.Analyzers/RegistrationExpectedInASPNetIntegration.cs Outdated
Copy link
Copy Markdown
Member

@fabiocav fabiocav left a comment

Choose a reason for hiding this comment

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

Haven't reviewed the implementation, but we need to make sure this analyzer is part of the ASP.NET Core integration package.

Comment thread extensions/Worker.Extensions.Http.AspNetCore.Analyzers/DiagnosticDescriptors.cs Outdated
Copy link
Copy Markdown
Contributor

@jviau jviau left a comment

Choose a reason for hiding this comment

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

I think we discussed having this analyzer only trigger if ConfigureFunctionsWorkerDefaults is present and ConfigureFunctionsWebApplication is not. Are we going with that approach or not?

I don't like the idea of having this analyzer trying to warn on the absence of something on IHostBuilder without something extra to indicate this is intended to be a function app and they are not configuring at as we expect.

@surgupta-msft
Copy link
Copy Markdown
Contributor Author

surgupta-msft commented Oct 23, 2023

I think we discussed having this analyzer only trigger if ConfigureFunctionsWorkerDefaults is present and ConfigureFunctionsWebApplication is not. Are we going with that approach or not?

I don't like the idea of having this analyzer trying to warn on the absence of something on IHostBuilder without something extra to indicate this is intended to be a function app and they are not configuring at as we expect.

(Providing a brief summary below about the things we discussed to avoid any confusion)

My understanding is we changed error to warning because absence of ConfigureFunctionsWebApplication() may not mean that the app is configured incorrectly and it won't work at all. The other thing we discussed which is tracked by a separate issue #1947 is the presence of both methods which can cause issues due to redundant calls. For that, we discussed that if we fix the redundant calls issue then we may not need any analyzer for that case at all.

For this exact thing - this analyzer only trigger if ConfigureFunctionsWorkerDefaults is present and ConfigureFunctionsWebApplication is not. I will setup some time to close on this.

Update - had an offline discussion. I have updated the code with this check. Thanks for catching this.

Comment thread docs/analyzer-rules/AZFW0014.md
@surgupta-msft surgupta-msft merged commit 54dbe73 into main Oct 25, 2023
@surgupta-msft surgupta-msft deleted the surgupta/aspnet-analyzer branch October 25, 2023 23:51
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.

Create analyzer to detect missing ASP.NET Core integration registration

5 participants