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

fix!: rename namespace, add OpenFeature dep #18

Merged
merged 9 commits into from
Oct 16, 2022
Merged

Conversation

toddbaert
Copy link
Member

@toddbaert toddbaert commented Oct 14, 2022

This is mostly just renames to prevent an issue @benjiro experienced earlier with the SDK where the OpenFeature namespace collided with the singleton. I've updated the namespaces and dirs to OpenFeatureContrib. It also adds a README.

The other thing I've done is add a PackageRef for the SDK:

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

I'm not sure if I need to add exclude/include/privateassets attributes... I want the library I'm releasing to use the version the consumer provides, not a transatively resolved version. I'm not sure exactly how to express this.

@toddbaert toddbaert force-pushed the fix/naming-and-deps branch 3 times, most recently from ad2543b to 9db277a Compare October 14, 2022 20:55
Comment on lines 2 to 6

<ItemGroup>
<PackageReference Include="OpenFeature" Version="$(OpenFeatureVer)">
<PrivateAssets>runtime;contentfiles;analyzers;build</PrivateAssets>
</PackageReference>
</ItemGroup>

Copy link
Member Author

@toddbaert toddbaert Oct 14, 2022

Choose a reason for hiding this comment

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

I'm having a difficult time understanding the documentation here: https://learn.microsoft.com/en-us/nuget/consume-packages/package-references-in-project-files

Is this right? I've basically used the default PrivateAssets, plus added runtime

The resultant nuspec dependencies element looks like this:

    <dependencies>
      <group targetFramework=".NETFramework4.6.2">
        <dependency id="OpenFeature" version="[0.4.0, 0.5.0)" exclude="Runtime,Build,Analyzers" />
      </group>
      <group targetFramework=".NETStandard2.0">
        <dependency id="OpenFeature" version="[0.4.0, 0.5.0)" exclude="Runtime,Build,Analyzers" />
      </group>
    </dependencies>

Copy link
Member

Choose a reason for hiding this comment

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

@toddbaert I don't think we need to worry about playing around with asset inclusion, the default behaviour is to include all.

The current versioning will resolve the smallest acceptable version (0.4.0) unless the consumer already has a higher version dependency on OpenFeature which nuget will resolve that version instead (anything less than 0.5.0). We probably want it to automatically take the latest patch version which would be 0.4.* https://learn.microsoft.com/en-us/nuget/concepts/package-versioning#version-ranges

Hope that makes sense.

Copy link
Member Author

@toddbaert toddbaert Oct 15, 2022

Choose a reason for hiding this comment

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

@toddbaert I don't think we need to worry about playing around with asset inclusion, the default behaviour is to include all.

OK, I've removed the customization of the asset inclusion.

The current versioning will resolve the smallest acceptable version (0.4.0) unless the consumer already has a higher version dependency on OpenFeature which nuget will resolve that version instead (anything less than 0.5.0). We probably want it to automatically take the latest patch version which would be 0.4.* https://learn.microsoft.com/en-us/nuget/concepts/package-versioning#version-ranges

Hope that makes sense.

It does.

However, if I modify the value of the OpenFeatureVer as you specify:

<OpenFeatureVer>0.4.*</OpenFeatureVer>

Then the resultant nuspec dependency element looks like this:

    <dependencies>
      <group targetFramework=".NETFramework4.6.2">
        <dependency id="OpenFeature" version="0.4.0" />
      </group>
      <group targetFramework=".NETStandard2.0">
        <dependency id="OpenFeature" version="0.4.0" />
      </group>
    </dependencies>

As you can see, it's a specific version... 0.4.0. I think we'd want this to be a range, no?

Copy link
Member

Choose a reason for hiding this comment

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

Yea ok, seems like nuget doesn't allow it without repackage the project each time a new version is release.

Lets just keep with the version range, if the consumer wants the latest patch version of OpenFeature they will need to take a direct dependency to override the transitive one.

DotnetSdkContribs.sln Outdated Show resolved Hide resolved
build/Common.props Outdated Show resolved Hide resolved
Comment on lines 2 to 6

<ItemGroup>
<PackageReference Include="OpenFeature" Version="$(OpenFeatureVer)">
<PrivateAssets>runtime;contentfiles;analyzers;build</PrivateAssets>
</PackageReference>
</ItemGroup>

Copy link
Member

Choose a reason for hiding this comment

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

@toddbaert I don't think we need to worry about playing around with asset inclusion, the default behaviour is to include all.

The current versioning will resolve the smallest acceptable version (0.4.0) unless the consumer already has a higher version dependency on OpenFeature which nuget will resolve that version instead (anything less than 0.5.0). We probably want it to automatically take the latest patch version which would be 0.4.* https://learn.microsoft.com/en-us/nuget/concepts/package-versioning#version-ranges

Hope that makes sense.

@toddbaert toddbaert force-pushed the fix/naming-and-deps branch from 90c5855 to 00bf340 Compare October 15, 2022 00:55
@toddbaert toddbaert requested a review from benjiro October 15, 2022 01:42
@toddbaert toddbaert force-pushed the fix/naming-and-deps branch from 6413687 to 0aa3b8d Compare October 15, 2022 01:44
@toddbaert toddbaert requested a review from beeme1mr October 15, 2022 01:45
@toddbaert toddbaert force-pushed the fix/naming-and-deps branch 2 times, most recently from 1003a25 to d71e348 Compare October 15, 2022 01:52
Copy link
Member

@benjiro benjiro left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

build/Common.props Outdated Show resolved Hide resolved
toddbaert and others added 7 commits October 15, 2022 08:36
@toddbaert toddbaert force-pushed the fix/naming-and-deps branch from 43873be to 264168b Compare October 15, 2022 12:36
@toddbaert
Copy link
Member Author

@benjiro I know it's a breaking change, and it would be a bit annoying, but what do you think about renaming the OpenFeature singleton to "Api" and changing the namespaces in both the contribs and the SDK to "OpenFeature"?

We certainly don't have to, I'm just trying to make sure we make the optional decision before a 1.0. Let me know your thoughts.

@benjiro
Copy link
Member

benjiro commented Oct 15, 2022

Yea I think that is a better way to go, I think the Java SDK did something similar from memory.

@toddbaert
Copy link
Member Author

Yea I think that is a better way to go, I think the Java SDK did something similar from memory.

Cool. I can do this but it won't be until later today/tonight.

@benjiro
Copy link
Member

benjiro commented Oct 15, 2022

@toddbaert check out open-feature/dotnet-sdk#82, im out all day if it needs to be changed go ahead

Signed-off-by: Todd Baert <[email protected]>
@toddbaert
Copy link
Member Author

@toddbaert check out open-feature/dotnet-sdk#82, im out all day if it needs to be changed go ahead

Amazing! I just pushed a fix for a formatting issue in your SDK PR. Published now and looks good. I absorbed the 0.5.0 changes here and they look great. I'm much happier with this naming! Thank you again.

Signed-off-by: Todd Baert <[email protected]>
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.

2 participants