open-feature / dotnet-sdk

.NET implementation of the OpenFeature SDK
https://openfeature.dev
Apache License 2.0
69 stars 19 forks source link

chore: Add support for GitHub Packages #173

Closed austindrenski closed 9 months ago

austindrenski commented 9 months ago
GITHUB_REF version format
refs/heads/* #.#.#-ci.{%Y%m%d}T{%H%M%S}+sha.${GITHUB_SHA:0:9}
refs/pull/* #.#.#-pr.{%Y%m%d}T{%H%M%S}+sha.${GITHUB_SHA:0:9}

See: #54, open-feature/dotnet-sdk-contrib#134

Closes: #54

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (f47cf07) 0.00% compared to head (0a0273d) 93.86%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #173 +/- ## ========================================= + Coverage 0 93.86% +93.86% ========================================= Files 0 23 +23 Lines 0 946 +946 Branches 0 105 +105 ========================================= + Hits 0 888 +888 - Misses 0 34 +34 - Partials 0 24 +24 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

austindrenski commented 9 months ago

Well, still failed, but now I'm pretty sure it's just the fork issue. Assuming ${{ secrets.NUGET_TOKEN }} is available to others, then this will work as is, otherwise, I can update the conditions to skip publishing from forks.

benjiro commented 9 months ago

FYI the secrets.NUGET_TOKEN is for nuget.org registry but looking at this change its pushing to github registry. Also i believe forks won't have access to any secrets from memory. You might need to update the permissions on the workflow to get write access to github registeries on the secrets.GITHUB_TOKEN that the action runner generates https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs#defining-access-for-the-github_token-scopes

One problem with using githubs nuget registry is you need a github account setup to pull down the packages (this could of change since last time i checked). This is why i originally suggested using something like MyGet which offer free accounts for OpenSource projects.

I think gating the prerelease packages behind auth defeats the purpose, and makes for a cumbersome developer experience. Happy to go a head with it if people are fine with that.

austindrenski commented 9 months ago

FYI the secrets.NUGET_TOKEN is for nuget.org registry but looking at this change its pushing to github registry. Also i believe forks won't have access to any secrets from memory. You might need to update the permissions on the workflow to get write access to github registeries on the secrets.GITHUB_TOKEN that the action runner generates https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs#defining-access-for-the-github_token-scopes

Bah, I knew there was something I was forgetting. Thanks, will update.

One problem with using githubs nuget registry is you need a github account setup to pull down the packages (this could of change since last time i checked). This is why i originally suggested using something like MyGet which offer free accounts for OpenSource projects.

I think gating the prerelease packages behind auth defeats the purpose, and makes for a cumbersome developer experience. Happy to go a head with it if people are fine with that.

Full agree that GitHub should just give us the option to allow un-auth'd package retrieval for OSS projects, but I also don't think the auth requirement is all that big of a blocker.

Now this could be some corporate dev bias, but of all the authenticated feeds I have to deal with at my day job, GitHub Packages is the least annoying of them lol.

But bigger picture, I think providing prerelease packages directly from GitHub is a really useful solution for discoverability, plus if you're already setup to consume one GitHub package feed, then that auth carries over to the N+1 feed too.

So wouldn't ever recommend we only publish to GitHub Packages (e.g. as opposed to, say, nuget.org), but I think it's a really nice built-in option that comes pretty much for free (as in beer/time) and offers a nice UX in terms of code+packages+releases all in one place.

(also, none of what I'm saying here precludes also publishing to myget in addition to github, I'm just saying there's no reason to not offer github while we're at it.)

austindrenski commented 9 months ago

I think this is wrong. I could be missing something, but pull_request_target runs in the context of the base branch - so this will make all our CI tests run the code in main, not the code in whatever PR.

Geeze, I think you're right. I'm not sure what I was doing when I changed that (testing token perms, maybe?).

Reverting now.

austindrenski commented 9 months ago

Screencap for future readers showing where to find the uploaded NuGet packages for PR builds from forks:

image

Clicking on the nupkgs artifact will trigger a download of nupkgs.zip which should contain any packages resulting from the workflow:

image

image

austindrenski commented 9 months ago

Actually, hold on, don't merge this just yet @toddbaert. The uploaded package doesn't look like it included the version suffix. Reviewing now.

austindrenski commented 9 months ago

Actually, hold on, don't merge this just yet @toddbaert. The uploaded package doesn't look like it included the version suffix. Reviewing now.

Okay, I'd overlooked that build/Common.prod.props was using <Version> instead of <VersionPrefix>.

tldr; you can use either, but if you use <VersionPrefix> then you can supply <VersionSuffix> from the CLI via --version-suffix and MSBuild will do something along the lines of:

<Version Condition="'$(Version)' == '' and '$(VersionSuffix)' == ''">$(VersionPrefix)</Version>
<Version Condition="'$(Version)' == '' and '$(VersionSuffix)' != ''">$(VersionPrefix)-$(VersionSuffix)</Version>
diff --git a/build/Common.prod.props b/build/Common.prod.props
index c9e5855..383da6f 100644
--- a/build/Common.prod.props
+++ b/build/Common.prod.props
@@ -19,7 +19,7 @@
     <PackageLicenseExpression>Apache-2.0</PackageLicenseExpression>
     <Authors>OpenFeature Authors</Authors>
     <PackageRequireLicenseAcceptance>true</PackageRequireLicenseAcceptance>
-    <Version>$(VersionNumber)</Version>
+    <VersionPrefix>$(VersionNumber)</VersionPrefix>
     <AssemblyVersion>$(VersionNumber)</AssemblyVersion>
     <FileVersion>$(VersionNumber)</FileVersion>
   </PropertyGroup>
austindrenski commented 9 months ago

Resultant version of that last CI run looks better, but still concerned that the Deterministic + SourceLink checks are failing:

hiding this because it's rendering enormously in the web ui ![image](https://github.com/open-feature/dotnet-sdk/assets/21338699/9dbe6946-dad3-4d01-b8e9-5c3c0841f8af)
austindrenski commented 9 months ago

Okay, not entirely sure why it caused issues, but removing these since they're now set at the top of the file:

diff --git a/build/Common.prod.props b/build/Common.prod.props
index 383da6f..4d073ec 100644
--- a/build/Common.prod.props
+++ b/build/Common.prod.props
@@ -35,11 +35,4 @@
     <SymbolPackageFormat>snupkg</SymbolPackageFormat>
   </PropertyGroup>

-  <ItemGroup Condition="'$(Deterministic)'=='true'">
-    <SourceRoot Include="$(MSBuildThisFileDirectory)/" />
-  </ItemGroup>
-
-  <PropertyGroup Condition="'$(Deterministic)'=='true'">
-    <ContinuousIntegrationBuild>true</ContinuousIntegrationBuild>
-  </PropertyGroup>
 </Project>

Local packing still results in the expected source root/paths:

image

austindrenski commented 9 months ago

Woohoo, it lives! 🥳🥳🥳

image