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

IDownstreamDependencyMetadataManager accessibility #4073

Closed
RussKie opened this issue Jun 14, 2023 · 4 comments
Closed

IDownstreamDependencyMetadataManager accessibility #4073

RussKie opened this issue Jun 14, 2023 · 4 comments
Assignees

Comments

@RussKie
Copy link
Member

RussKie commented Jun 14, 2023


This interface is being used by unit tests in Microsoft.Azure.Extensions.Telemetry.Test. To facilitate that Microsoft.Extensions.Telemetry declares IVT to Microsoft.Azure.Extensions.Telemetry.Tests. This is... bad.

This IVT causes compiler errors in Azure/dotnet-extensions-experimental repo when an attempt to build it without signing:

D:\Development\azure-dotnet-extensions-experimental\tests\Microsoft.Azure.Extensions.Telemetry.Tests\AzureMetadataTests.cs(16,22): error CS0281: Friend access was granted by 'Microsoft.Extensions.Telemetry, Version=8.0.0.0, Culture=neutral, PublicKeyTok 
en=31bf3856ad364e35', but the public key of the output assembly ('') does not match that specified by the InternalsVisibleTo attribute in the granting assembly. [D:\Development\azure-dotnet-extensions-experimental\tests\Microsoft.Azure.Extensions.Teleme 
try.Tests\Microsoft.Azure.Extensions.Telemetry.Tests.csproj::TargetFramework=net8.0]

#4072 removes the IVT, however, this change will break the AzureMetadataTests tests.
image

I see the tests are being actively developed (e.g., https://domoreexp.visualstudio.com/R9/_git/SDK/pullrequest/782810).
@geeknoid @mariamgerges please advise the best course of action here. Do we need to make IDownstreamDependencyMetadataManager public?

@geeknoid
Copy link
Member

I don't want to make that type public, it's an implementation detail. I'm hoping there's a way to test the Azure stuff without needing this type.

@dpk83 ?

@RussKie
Copy link
Member Author

RussKie commented Jun 14, 2023

With #4072 merged I'll be disabling the Microsoft.Azure.Extensions.Telemetry.Test tests.

@dpk83
Copy link

dpk83 commented Jun 15, 2023

We shouldn’t make it public, rather update tests to remove the dependency

@RussKie
Copy link
Member Author

RussKie commented Jun 15, 2023

@RussKie RussKie closed this as completed Jun 15, 2023
@ghost ghost removed the untriaged label Jun 15, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants