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.

Sign up for GitHub

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

chore: Enable Central Package Management (CPM) #136

Closed
wants to merge 1 commit into from
Closed

chore: Enable Central Package Management (CPM) #136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions Directory.Packages.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<Project>

<PropertyGroup>
<ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
</PropertyGroup>

<ItemGroup>
<PackageVersion Include="ConfigCat.Client" Version="9.0.0" />
Copy link
Member

Choose a reason for hiding this comment

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

A potential downside of this is that "provider-unique" dependencies like this "ConfigCat.Client" are now centrally maintained. That's not ideal, particularly for such cases because it doesn't benefit from the ownerships defined here.

Before this change, owners of the provider that uses ConfigCat.Client would be responsible for that dependency, and alerted to automated updates pertaining to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, that's an interesting point!

I don't have a pre-canned solution, but I bet we could add a workflow that reacts to dependabot/renovate PRs and tags component owners if the dependency being updated is referenced by their component's csproj.

Assuming we can live with a minor coverage gap for a little while, I'll see if I can hack something together this weekend.

Copy link
Contributor

Choose a reason for hiding this comment

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

For such cases we can omit the specific package from the Directoty.Packages.props and keep the reference only in the project file.
The only difference is the use of VersionOverride instead of Version.
So for ConfigCat provider, we would have this line in the csproj.

<PackageReference Include="ConfigCat.Client" VersionOverride="9.0.0" />

I don't know how bots will behave with such config though.

Copy link
Member

Choose a reason for hiding this comment

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

Very interesting and valid point. @austindrenski I remember playing around with the CPM and as far as I remember all projects within the solution needed to be using CPM as soon as it was enabled. Please correct me if I'm wrong.

Due to the nature of this repository, perhaps makes sense to keep the package versions managed by the project rather than a central place.

Copy link
Member

Choose a reason for hiding this comment

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

For dependencies like ConfigCat.Client, is there any reason we can't leave them only in the package that uses them, while maintaining only the common, less-specific deps centrally? This is basically what we do with the pom.xml setup in the java contrib.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we leave only the specific packages in each project, this PR adds no value, as we already have Common.props controlling versions centrally.

Is it worth moving to CPM?
I don't see any specific project referencing the same package outside Common.props.

We would have value if more projects started to use common packages, but not all of them. Even though those packages are not supposed to be used together, having different dependency versions could be fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty sure they've added relief valves to make it less of an all-or-nothing, one such being the VersionOverride="..." mentioned by @luizbon, so I don't think the answer is to pull back from CPM, just a matter of tweaking a few things.

My order of preferences here would be:

I started writing up a couple of options, but after typing up the alternatives, I think there's one solution that'll cover all of our bases/concerns:

  • Directory.Packages.props remains the authoritative source for dependency versions
  • We define component-specific <ItemGroup>'s for component-specific dependencies
  • VersionOverride="..." remains available for use as a relief valve
    • e.g. in the event one provider needs to pin to a specific dependency that conflicts with the rest of the repo
    • i.e. if a dependency is only used once, it lives in Directory.Packages.props; if a common/shared dependency is used by two or more components and there's a version conflict, the lower of the versions is pinned where necessary with VersionOverride="..." (with the goal being to keep dependencies up to date and discourage long-term pinning)
  • @austindrenski puts together a workflow step (e.g. combination of gh and grep) to
    1. tag all component owners when a PR updates a common/shared dependency, and
    2. tag component-specific owners when a component-specific dependency is updated
      • e.g.:
        1. resolve Label attr from the dependency's parent <ItemGroup>
        2. lookup component owners in .github/component_owners.yml using the key from (1)
        3. gh pr edit "$PR_NUMBER" --add-assignee "$(gh pr view "$PR_NUMBER" --json author --jq .author.login)"

So in practice, here's what the Directory.Packages.props would look like today (i.e. as of this PR):

<Project>

  <PropertyGroup>
    <ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
  </PropertyGroup>

  <ItemGroup Label="src">
    <PackageVersion Include="Microsoft.SourceLink.GitHub" Version="[1.0.0, 2.0)" />
    <PackageVersion Include="OpenFeature" Version="[1.2,)" />
    <PackageVersion Include="OpenTelemetry.Api" Version="1.4.0" />
    <PackageVersion Include="System.Text.Json" Version="8.0.1" />
  </ItemGroup>

  <ItemGroup Label="src/OpenFeature.Contrib.Providers.ConfigCat">
    <PackageVersion Include="ConfigCat.Client" Version="9.0.0" />
  </ItemGroup>

  <ItemGroup Label="src/OpenFeature.Contrib.Providers.FeatureManagement">
    <PackageVersion Include="Microsoft.FeatureManagement" Version="4.0.0-preview" />
  </ItemGroup>

  <ItemGroup Label="src/OpenFeature.Contrib.Providers.Flagd">
    <PackageVersion Include="Google.Protobuf" Version="3.23.4" />
    <PackageVersion Include="Grpc.Net.Client" Version="2.59.0" />
    <PackageVersion Include="Grpc.Tools" Version="2.60.0" />
    <PackageVersion Include="System.Net.Http.WinHttpHandler" Version="8.0.0" />
  </ItemGroup>

  <ItemGroup Label="src/OpenFeature.Contrib.Providers.Flagsmith">
    <PackageVersion Include="Flagsmith" Version="5.2.2" />
  </ItemGroup>

  <ItemGroup Label="test">
    <PackageVersion Include="AutoFixture" Version="4.17.0" />
    <PackageVersion Include="AutoFixture.Xunit2" Version="4.17.0" />
    <PackageVersion Include="coverlet.collector" Version="3.1.2" />
    <PackageVersion Include="coverlet.msbuild" Version="3.1.2" />
    <PackageVersion Include="FluentAssertions" Version="6.7.0" />
    <PackageVersion Include="Grpc" Version="2.46.6" />
    <PackageVersion Include="Microsoft.Extensions.Configuration.Json" Version="8.0.0" />
    <PackageVersion Include="Microsoft.Extensions.DependencyInjection" Version="8.0.0" />
    <PackageVersion Include="Microsoft.NET.Test.Sdk" Version="17.3.2" />
    <PackageVersion Include="NSubstitute" Version="5.0.0" />
    <PackageVersion Include="OpenTelemetry" Version="1.4.0" />
    <PackageVersion Include="OpenTelemetry.Exporter.InMemory" Version="1.4.0" />
    <PackageVersion Include="RichardSzalay.MockHttp" Version="6.0.0" />
    <PackageVersion Include="xunit" Version="[2.4.1, 3.0)" />
    <PackageVersion Include="xunit.runner.visualstudio" Version="[2.4.3, 3.0)" />
  </ItemGroup>

</Project>

then we'd wire up the reviews using something like:

name: Request reviews

on:
  pull_request_target:

permissions:
  contents: read
  pull-requests: write

jobs:
  triage:
    runs-on: ubuntu-latest

    steps:
    - name: Request component-specific reviews
      env:
        GH_REPO: ${{ github.repository }}
        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
        PR_NUMBER: ${{ github.event.pull_request.number }}
      uses: mikefarah/yq@v4.40.5
      with:
        cmd: |
          # TODO: resolve `Label` attr from the dependency's parent `<ItemGroup>`
          COMPONENT=$(...)
          gh pr edit "PR_NUMBER" --add-reviewer "$(gh api /repos/{owner}/{repo}/contents/.github/component_owners.yml --jq .content | base64 -d | yq -o csv .components.[\"$COMPONENT\"])"

Copy link
Member Author

Choose a reason for hiding this comment

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

@luizbon wrote (emphasis added):

[...]

If we leave only the specific packages in each project, this PR adds no value, as we already have Common.props controlling versions centrally.

Is it worth moving to CPM? I don't see any specific project referencing the same package outside Common.props.

[...]

Didn't see this before posting my previous reply, but wanted to respond to this specifically:

I hear where you're coming from on this, but when it comes to repo engineering, I think there's value in using standard mechanisms whenever possible, even if they don't necessarily provide "more" than what our custom mechanism does.

So a couple of years ago, I would've been all for the Common.props pattern, because it definitely beats tracking versions in each csproj, but in a post-CPM world, I have a growing preference for just using the built-in features where possible/practical.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good discussion @austindrenski.

Either approach will achieve the goal.

However, I don't see the use of Common.props as a non-standard way of tracking package versions. It is a supported solution to stop the repetition across projects, and the version is one of them.

Indeed, CPM is a more elegant/modern solution for package control, but this specific project structure adds more complexity than solves the problem.
Adding more customisation to the workflow is proof of that.

Sorry to say, but I still don't see value in adding CPM to this repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Closing the loop here: yes, you are all correct, and I'm afraid I just outright misunderstood the governance of this repo 🤦🤦🤦

Given that this repo is for co-hosting/-locating contributor-maintained projects that should be able to evolve and release independently, this PR is indeed the wrong approach.

Thanks for your patience while I caught up with what you were saying @luizbon and @toddbaert 🙇🙇🙇

<PackageVersion Include="Flagsmith" Version="5.2.2" />
<PackageVersion Include="Google.Protobuf" Version="3.23.4" />
<PackageVersion Include="Grpc" Version="2.46.6" />
<PackageVersion Include="Grpc.Net.Client" Version="2.59.0" />
<PackageVersion Include="Grpc.Tools" Version="2.60.0" />
<PackageVersion Include="Microsoft.Extensions.Configuration.Json" Version="8.0.0" />
<PackageVersion Include="Microsoft.Extensions.DependencyInjection" Version="8.0.0" />
<PackageVersion Include="Microsoft.FeatureManagement" Version="4.0.0-preview" />
<PackageVersion Include="Microsoft.SourceLink.GitHub" Version="[1.0.0, 2.0)" />
<PackageVersion Include="OpenFeature" Version="[1.2,)" />
<PackageVersion Include="OpenTelemetry" Version="1.4.0" />
<PackageVersion Include="OpenTelemetry.Api" Version="1.4.0" />
<PackageVersion Include="OpenTelemetry.Exporter.InMemory" Version="1.4.0" />
<PackageVersion Include="System.Net.Http.WinHttpHandler" Version="8.0.0" />
<PackageVersion Include="System.Text.Json" Version="8.0.1" />
</ItemGroup>

<ItemGroup>
<PackageVersion Include="AutoFixture" Version="4.17.0" />
<PackageVersion Include="AutoFixture.Xunit2" Version="4.17.0" />
<PackageVersion Include="coverlet.collector" Version="3.1.2" />
<PackageVersion Include="coverlet.msbuild" Version="3.1.2" />
<PackageVersion Include="FluentAssertions" Version="6.7.0" />
<PackageVersion Include="Microsoft.NET.Test.Sdk" Version="17.3.2" />
<PackageVersion Include="NSubstitute" Version="5.0.0" />
<PackageVersion Include="RichardSzalay.MockHttp" Version="6.0.0" />
<PackageVersion Include="xunit" Version="[2.4.1, 3.0)" />
<PackageVersion Include="xunit.runner.visualstudio" Version="[2.4.3, 3.0)" />
</ItemGroup>

</Project>
14 changes: 2 additions & 12 deletions build/Common.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project>

<ItemGroup>
<PackageReference Include="OpenFeature" Version="$(OpenFeatureVer)" />
<PackageReference Include="OpenFeature" />
</ItemGroup>

<PropertyGroup>
Expand All @@ -17,14 +17,4 @@
<PropertyGroup Condition="'$(Configuration)'=='Release'">
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
</PropertyGroup>

<PropertyGroup Label="Package versions used in this repository">
<!--
Please sort alphabetically.
Refer to https://docs.microsoft.com/nuget/concepts/package-versioning for semver syntax.
-->
<MicrosoftSourceLinkGitHubPkgVer>[1.0.0,2.0)</MicrosoftSourceLinkGitHubPkgVer>
<!-- 0.5+ -->
<OpenFeatureVer>[1.2,)</OpenFeatureVer>
</PropertyGroup>
</Project>
</Project>
30 changes: 8 additions & 22 deletions build/Common.tests.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,36 +18,22 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="AutoFixture" Version="$(AutoFixtureVer)" />
<PackageReference Include="coverlet.collector" Version="$(CoverletCollectorVer)">
<PackageReference Include="AutoFixture" />
<PackageReference Include="coverlet.collector">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
<PackageReference Include="coverlet.msbuild" Version="$(CoverletCollectorVer)">
<PackageReference Include="coverlet.msbuild">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
<PackageReference Include="FluentAssertions" Version="$(FluentAssertionsVer)" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="$(MicrosoftNETTestSdkPkgVer)" />
<PackageReference Include="NSubstitute" Version="$(NSubstituteVer)" />
<PackageReference Include="xunit" Version="$(XUnitPkgVer)" />
<PackageReference Include="xunit.runner.visualstudio" Version="$(XUnitRunnerVisualStudioPkgVer)">
<PackageReference Include="FluentAssertions" />
<PackageReference Include="Microsoft.NET.Test.Sdk" />
<PackageReference Include="NSubstitute" />
<PackageReference Include="xunit" />
<PackageReference Include="xunit.runner.visualstudio">
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
<PrivateAssets>all</PrivateAssets>
</PackageReference>
</ItemGroup>

<PropertyGroup Label="Package versions used in this repository">
<!--
Please sort alphabetically.
Refer to https://docs.microsoft.com/nuget/concepts/package-versioning for semver syntax.
-->
<AutoFixtureVer>[4.17.0]</AutoFixtureVer>
<CoverletCollectorVer>[3.1.2]</CoverletCollectorVer>
<FluentAssertionsVer>[6.7.0]</FluentAssertionsVer>
<MicrosoftNETTestSdkPkgVer>[17.3.2]</MicrosoftNETTestSdkPkgVer>
<NSubstituteVer>[5.0.0]</NSubstituteVer>
<XUnitRunnerVisualStudioPkgVer>[2.4.3,3.0)</XUnitRunnerVisualStudioPkgVer>
<XUnitPkgVer>[2.4.1,3.0)</XUnitPkgVer>
</PropertyGroup>
</Project>
2 changes: 1 addition & 1 deletion src/Directory.Build.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<Project>
<ItemGroup>
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="$(MicrosoftSourceLinkGitHubPkgVer)">
<PackageReference Include="Microsoft.SourceLink.GitHub">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
</PackageReference>
Expand Down
2 changes: 1 addition & 1 deletion src/OpenFeature.Contrib.Hooks.Otel/OpenFeature.Contrib.Hooks.Otel.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="OpenTelemetry.Api" Version="1.4.0" />
<PackageReference Include="OpenTelemetry.Api" />
</ItemGroup>

</Project>
4 changes: 2 additions & 2 deletions src/OpenFeature.Contrib.Providers.ConfigCat/OpenFeature.Contrib.Providers.ConfigCat.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@
</AssemblyAttribute>
</ItemGroup>
<ItemGroup>
<PackageReference Include="ConfigCat.Client" Version="[9,)"/>
<PackageReference Include="ConfigCat.Client" />
</ItemGroup>
</Project>
</Project>
4 changes: 2 additions & 2 deletions ...ontrib.Providers.FeatureManagement/OpenFeature.Contrib.Providers.FeatureManagement.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.FeatureManagement" Version="4.0.0-preview" />
<PackageReference Include="Microsoft.FeatureManagement" />
</ItemGroup>

</Project>
</Project>
8 changes: 4 additions & 4 deletions src/OpenFeature.Contrib.Providers.Flagd/OpenFeature.Contrib.Providers.Flagd.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,15 @@
</AssemblyAttribute>
</ItemGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'net462'">
<PackageReference Include="System.Net.Http.WinHttpHandler" Version="8.0.0" />
<PackageReference Include="System.Net.Http.WinHttpHandler" />
</ItemGroup>
<ItemGroup>
<!-- The schema.proto file referenced here will be used to automatically generate the Grpc client when executing 'dotnet build' -->
<!-- The generated files will be placed in ./obj/Debug/netstandard2.0/Protos -->
<Protobuf Include="schemas\protobuf\schema\v1\schema.proto" GrpcServices="Client" />
<PackageReference Include="Google.Protobuf" Version="3.23.4" />
<PackageReference Include="Grpc.Net.Client" Version="2.59.0" />
<PackageReference Include="Grpc.Tools" Version="2.60.0">
<PackageReference Include="Google.Protobuf" />
<PackageReference Include="Grpc.Net.Client" />
<PackageReference Include="Grpc.Tools">
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
<PrivateAssets>all</PrivateAssets>
</PackageReference>
Expand Down
4 changes: 2 additions & 2 deletions src/OpenFeature.Contrib.Providers.Flagsmith/OpenFeature.Contrib.Providers.Flagsmith.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="Flagsmith" Version="5.2.2" />
<PackageReference Include="System.Text.Json" Version="8.0.1" />
<PackageReference Include="Flagsmith" />
<PackageReference Include="System.Text.Json" />
</ItemGroup>

<PropertyGroup>
Expand Down
2 changes: 1 addition & 1 deletion ...eature.Contrib.Providers.GOFeatureFlag/OpenFeature.Contrib.Providers.GOFeatureFlag.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="System.Text.Json" Version="8.0.1" />
<PackageReference Include="System.Text.Json" />
</ItemGroup>

<PropertyGroup>
Expand Down
6 changes: 3 additions & 3 deletions test/OpenFeature.Contrib.Hooks.Otel.Test/OpenFeature.Contrib.Hooks.Otel.Test.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
<ProjectReference Include="..\..\src\OpenFeature.Contrib.Hooks.Otel\OpenFeature.Contrib.Hooks.Otel.csproj" />
</ItemGroup>

<ItemGroup>
<PackageReference Include="OpenTelemetry" Version="1.4.0" />
<PackageReference Include="OpenTelemetry.Exporter.InMemory" Version="1.4.0" />
<ItemGroup>
<PackageReference Include="OpenTelemetry" />
<PackageReference Include="OpenTelemetry.Exporter.InMemory" />
</ItemGroup>

</Project>
4 changes: 2 additions & 2 deletions ...ture.Contrib.Providers.ConfigCat.Test/OpenFeature.Contrib.Providers.ConfigCat.Test.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
<Project Sdk="Microsoft.NET.Sdk">

<ItemGroup>
<PackageReference Include="AutoFixture.Xunit2" Version="4.17.0" />
<PackageReference Include="AutoFixture.Xunit2" />
<ProjectReference Include="..\..\src\OpenFeature.Contrib.Providers.ConfigCat\OpenFeature.Contrib.Providers.ConfigCat.csproj" />
</ItemGroup>

</Project>
</Project>
4 changes: 2 additions & 2 deletions ...viders.FeatureManagement.Test/OpenFeature.Contrib.Providers.FeatureManagement.Test.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="8.0.0" />
<PackageReference Include="Microsoft.Extensions.DependencyInjection" Version="8.0.0" />
<PackageReference Include="Microsoft.Extensions.Configuration.Json" />
<PackageReference Include="Microsoft.Extensions.DependencyInjection" />
</ItemGroup>

<ItemGroup>
Expand Down
4 changes: 2 additions & 2 deletions .../OpenFeature.Contrib.Providers.Flagd.Test/OpenFeature.Contrib.Providers.Flagd.Test.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="Grpc" Version="2.46.6" />
<PackageReference Include="Grpc" />
</ItemGroup>

</Project>
</Project>
2 changes: 1 addition & 1 deletion ...trib.Providers.GOFeatureFlag.Test/OpenFeature.Contrib.Providers.GOFeatureFlag.Test.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@
<ProjectReference Include="..\..\src\OpenFeature.Contrib.Providers.GOFeatureFlag\OpenFeature.Contrib.Providers.GOFeatureFlag.csproj" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="RichardSzalay.MockHttp" Version="6.0.0" />
<PackageReference Include="RichardSzalay.MockHttp" />
</ItemGroup>
</Project>
Loading