chore: Enable Central Package Management (CPM)

[austindrenski]

See: open-feature/dotnet-sdk#178

[askpt]

Always liked how clean the CPM makes the dependencies look like.

[toddbaert]

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.

[austindrenski]

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.

[luizbon]

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.

[askpt]

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.

[toddbaert]

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.

[luizbon]

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.

[austindrenski]

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\"])"

[austindrenski]

@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.

[luizbon]

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.

[austindrenski]

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 🙇🙇🙇

[austindrenski]

Closing after discussion because dependency isolation is by-design in contrib-style repos (and possibly something we should take further w.r.t. moving all dependency versions out of Common.prod.props).

See: #136 (review), #136 (comment)