Skip to content

Conversation

@xiang17
Copy link
Contributor

@xiang17 xiang17 commented Jan 19, 2023

Why

Fixes #1974: Centralize dependency package versions.

What

Use Central Package Management
https://devblogs.microsoft.com/nuget/introducing-central-package-management/

Note1: the top comment in the article above says it's already supported by GitHub’s Dependabot, but I haven't been able to verify that or found any article in any other source.

Note2: I left a TODO comment on whether it's better to consolidate all packages in one ItemGroup and sort them alphabetically. I'd appreciate the community's feedback!

Tests

CI

Checklist

- [ ] CHANGELOG.md is updated.
- [ ] Documentation is updated.
- [ ] New features are covered by tests.

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

Hi @xiang17 - I had just a few minutes to look at it, I will finish it later today. Meanwhile, a few questions.

# Conflicts:
#	src/OpenTelemetry.AutoInstrumentation/OpenTelemetry.AutoInstrumentation.csproj
#	test/IntegrationTests/IntegrationTests.csproj
@xiang17 xiang17 marked this pull request as ready for review January 20, 2023 01:44
@xiang17 xiang17 requested a review from a team January 20, 2023 01:44
@@ -0,0 +1,47 @@
<Project>
<Import Project="..\Directory.Packages.props" />
Copy link
Member

Choose a reason for hiding this comment

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

I think that it will be good idea to reference here src\Directory.Packages.props
but without the section related to CommonExcludedAssets.props.
(Mainly to have OpenTelemetry* defined only in one place),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that as well, but there are only four dependencies that are common:

# cat src/Directory.Packages.props test/Directory.Packages.props | sort | uniq -d | grep Version
    <PackageVersion Include="OpenTelemetry.Exporter.OpenTelemetryProtocol" Version="1.4.0-rc.2" />
    <PackageVersion Include="OpenTelemetry.Instrumentation.Http" Version="1.0.0-rc9.11" />
    <PackageVersion Include="OpenTelemetry.Instrumentation.Wcf" Version="1.0.0-rc.8" />
    <PackageVersion Include="OpenTelemetry" Version="1.4.0-rc.2" />

I've put them into Directory.Packages.props file in the root folder.

# Conflicts:
#	examples/demo/Service/Examples.Service.csproj
#	src/OpenTelemetry.AutoInstrumentation/OpenTelemetry.AutoInstrumentation.csproj
#	test/IntegrationTests/IntegrationTests.csproj
<PackageVersion Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.3" />
</ItemGroup>

<!-- Versions from OpenTelemetry.AutoInstrumentation.csproj -->
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment is wrong after uniting itemgroups.

Copy link
Contributor

@RassK RassK left a comment

Choose a reason for hiding this comment

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

Overall seems great!

@pellared pellared enabled auto-merge (squash) January 23, 2023 11:41
@pellared pellared merged commit a36ba28 into open-telemetry:main Jan 23, 2023
@pellared
Copy link
Member

After seeing PRs like this #2054 I think that too much has been moved to Central Package Management.

I think that the instrumented library version should be declared only in csproj.

@xiang17
Copy link
Contributor Author

xiang17 commented Jan 24, 2023

After seeing PRs like this #2054 I think that too much has been moved to Central Package Management.

I think that the instrumented library version should be declared only in csproj.

The purpose of the PR was to make it easier for upgrading versions, especially for shared dependencies used in multiple places. The opentelemetry-dotnet repo centralizes almost all versions because most of them are shared. However, the same might not apply to this repo seeing recent PRs.

I took a look at the recent dependabot PRs. Most of them are either in CommonExcludedAssets.props (9) or in tests/Directory.Packages.props (5).

The packages in tests/Directory.Packages.props file aren't shared much (the csproj files are in test/test-applications/integrations/). I can put them back.

Regarding CommonExcludedAssets.props, these seems like to be excluding packages of old versions not expected to be upgraded as new versions come by. I didn't manage to figure out how they got skipped by dependabot before. Shall I put the versions back to CommonExcludedAssets.props file as well?

@pellared
Copy link
Member

@xiang17 I created #2068

@xiang17 xiang17 deleted the xiang17/CentralPackageManagement branch April 13, 2023 21:32
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.

Centralize dependency package versions

6 participants