Skip to content
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

Add explicit reference to Microsoft.Bcl.AsyncInterfaces in HealthChecks #46920

Merged
merged 3 commits into from
Mar 3, 2023

Conversation

MackinnonBuck
Copy link
Member

Add explicit reference to Microsoft.Bcl.AsyncInterfaces in HealthChecks

Addresses #46683 by adding an explicit reference to Microsoft.Bcl.AsyncInterfaces from Microsoft.Extensions.Diagnostics.HealthChecks when the target framework is netstandard2.0.

Opening initially as a draft PR to gather feedback.

@MackinnonBuck MackinnonBuck requested a review from dougbu February 28, 2023 00:51
@dougbu dougbu requested a review from a team February 28, 2023 00:52
@dougbu
Copy link
Member

dougbu commented Feb 28, 2023

@JamesNK, @BrennanConroy, @captainsafia you've all made HealthChecks changes. But I'm not sure who the current "HealthCheck team members" are. Suggestions for reviewers / discussion participants❔

@captainsafia
Copy link
Member

@JamesNK, @BrennanConroy, @captainsafia you've all made HealthChecks changes. But I'm not sure who the current "HealthCheck team members" are. Suggestions for reviewers / discussion participants❔

I think tagging @dotnet/minimal-apis should be good for this area.

@dougbu dougbu requested a review from a team February 28, 2023 04:17
@captainsafia captainsafia force-pushed the mbuck/add-explicit-asyncinterfaces-ref branch from 1193f3a to 01bbe18 Compare March 2, 2023 17:00
@captainsafia captainsafia marked this pull request as ready for review March 2, 2023 17:00
@captainsafia captainsafia requested a review from wtgodbe as a code owner March 2, 2023 17:00
@captainsafia
Copy link
Member

I think this is read for review. @dougbu @wtgodbe Can you confirm that I've set up the package for automatic updates correctly?

Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

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

Looks good!

@dougbu
Copy link
Member

dougbu commented Mar 2, 2023

Agree this change looks good. However, it's necessary but not sufficient. Need to check projects for transitive dependencies (perhaps searching project.assets.json or *.deps.json files) for Microsoft.Bcl.* references that aren't explicit in the corresponding *.csproj files. That is, we need to find other indirect references that should be made direct. Doesn't have to be w/in this PR of course…

@captainsafia captainsafia force-pushed the mbuck/add-explicit-asyncinterfaces-ref branch from 01bbe18 to 6769fbe Compare March 2, 2023 22:36
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

I agree with @dougbu that we should automatically detect this issue, but fixing the known issue in the meantime still seems good.

@captainsafia captainsafia merged commit db7f79a into main Mar 3, 2023
@captainsafia captainsafia deleted the mbuck/add-explicit-asyncinterfaces-ref branch March 3, 2023 00:14
@ghost ghost added this to the 8.0-preview3 milestone Mar 3, 2023
@captainsafia
Copy link
Member

I agree with @dougbu that we should automatically detect this issue, but fixing the known issue in the meantime still seems good.

Yeah, I think this should be tracked as a separate infrastructure issue. For the moment, we've addressed the user reported instance.

@dougbu
Copy link
Member

dougbu commented Mar 3, 2023

I agree with @dougbu that we should automatically detect this issue, but fixing the known issue in the meantime still seems good.

Yeah, I think this should be tracked as a separate infrastructure issue. For the moment, we've addressed the user reported instance.

Agreed, mostly. I wasn't suggesting an ongoing check for indirect references to BCL packages, just a one-time search and another PR.

@captainsafia
Copy link
Member

I agree with @dougbu that we should automatically detect this issue, but fixing the known issue in the meantime still seems good.

Yeah, I think this should be tracked as a separate infrastructure issue. For the moment, we've addressed the user reported instance.

Agreed, mostly. I wasn't suggesting an ongoing check for indirect references to BCL packages, just a one-time search and another PR.

I see. I can give this a stab tomorrow.

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.

5 participants