ButterCMS / buttercms-csharp

.NET API client for ButterCMS (https://buttercms.com)
MIT License
40 stars 16 forks source link

chore: release-please action #40

Closed vlnevyhosteny closed 1 year ago

vlnevyhosteny commented 1 year ago

Release-As: 2.0.1

ViolanteCodes commented 1 year ago

@vlnevyhosteny - I've assigned my colleague, @Toreno96 to this review, as he has more experience with github actions and has looked at the other similar PRs; however, can you please update the commit message from "chore" to "feat" or "fix"? "chore" will not generate a new release with the release-please action.

vlnevyhosteny commented 1 year ago

@vlnevyhosteny - I've assigned my colleague, @Toreno96 to this review, as he has more experience with github actions and has looked at the other similar PRs; however, can you please update the commit message from "chore" to "feat" or "fix"? "chore" will not generate a new release with the release-please action.

@ViolanteCodes Ok, I will rebase commit to be "feat". However I think it's not really correct since these changes does not impact users that are using SDK. It's more infrastructure related.

Toreno96 commented 1 year ago

@ViolanteCodes Ok, I will rebase commit to be "feat". However I think it's not really correct since these changes does not impact users that are using SDK. It's more infrastructure related.

@vlnevyhosteny Can we somehow force release-please to deploy the chore change?

@ViolanteCodes alternatively, we could consider merging this as chore, skipping the deploy, and waiting until we have a change that actually should change the version. The disadvantage of that solution is that we won't be able to test it all works right away; the advantage is it won't update the version for something that does not impact the user as @vlnevyhosteny mentioned.

Toreno96 commented 1 year ago

@vlnevyhosteny One other concern I have—shouldn't we add an action (either the separate one or as a part of release-please.yml) that publishes the package to NuGet packages as instructed here? It seems to me that after this PR, it will only bump the version, but not publish it anywhere, correct?

ViolanteCodes commented 1 year ago

@ViolanteCodes alternatively, we could consider merging this as chore, skipping the deploy, and waiting until we have a change that actually should change the version. The disadvantage of that solution is that we won't be able to test it all works right away; the advantage is it won't update the version for something that does not impact the user as @vlnevyhosteny mentioned.

I am okay with this, as long as @vlnevyhosteny comes back if the deploy to nuget doesn't work 😉

ViolanteCodes commented 1 year ago

@vlnevyhosteny One other concern I have—shouldn't we add an action (either the separate one or as a part of release-please.yml) that publishes the package to NuGet packages as instructed here? It seems to me that after this PR, it will only bump the version, but not publish it anywhere, correct?

I agree with this - it's what we have for all the other SDK updates - they not only autoversion, but they then deploy out to whatever the package hosting service is

vlnevyhosteny commented 1 year ago

I agree with this - it's what we have for all the other SDK updates - they not only autoversion, but they then deploy out to whatever the package hosting service is

@ViolanteCodes @Toreno96 Hmm, I had info from @prokopsimek that for now just release is on the table and publish is not. 😅 Let me invite him to the discussion to clarify, please.

prokopsimek commented 1 year ago

I meant to be aware that all the publish actions will differ per a tech stack and only the release-please job will be probably the same. Sorry for confusion. Definitely add both jobs please. 👍😀

On October 5, 2023 at 16:59:08, vnevyhosteny @. @.)) wrote:

I agree with this - it's what we have for all the other SDK updates - they not only autoversion, but they then deploy out to whatever the package hosting service is

@ViolanteCodes (https://github.com/ViolanteCodes) @Toreno96 (https://github.com/Toreno96) Hmm, I had info from @prokopsimek (https://github.com/prokopsimek) that for now just release is on the table not publish. 😅 Let me invite him to the discussion to clarify, please.

— Reply to this email directly, view it on GitHub (https://github.com/ButterCMS/buttercms-csharp/pull/40#issuecomment-1749082241), or unsubscribe (https://github.com/notifications/unsubscribe-auth/ABJ3U4M65MTHFCFFWD3C3LTX53DLZAVCNFSM6AAAAAA5QRVOM2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONBZGA4DEMRUGE). You are receiving this because you were mentioned.Message ID: @.***>

vlnevyhosteny commented 1 year ago

I meant to be aware that all the publish actions will differ per a tech stack and only the release-please job will be probably the same. Sorry for confusion. Definitely add both jobs please. 👍😀

Ahh, my bad sorry. I will add that job then and let you know guys. 🙏🏻

vlnevyhosteny commented 1 year ago

@ViolanteCodes @Toreno96 So, in https://github.com/ButterCMS/buttercms-csharp/pull/40/commits/f2e4a865ea97785f7146bd83c4808969a15a6fd4 I have added workflow with publish to NuGET.

I have tested that on https://github.com/vlnevyhosteny/buttercms-csharp/tree/publish-testing and successfully published https://www.nuget.org/packages/Vlads.ButterCMS/ .

Just few things to mention:

Run meziantou.validate-nuget-package $(ls /home/runner/work/buttercms-csharp/buttercms-csharp/nuget/*.nupkg | head -n 1)
{
  "Packages": {
    "/home/runner/work/buttercms-csharp/buttercms-csharp/nuget/Vlads.ButterCMS.2.0.0.nupkg": {
      "Errors": [
        {
          "ErrorCode": 101,
          "Message": "XML documentation not found",
          "HelpText": "lib/netstandard2.1/ButterCMS.dll"
        },
        {
          "ErrorCode": 111,
          "Message": "Symbol file not found",
          "FileName": "lib/netstandard2.1/ButterCMS.dll"
        },
        {
          "ErrorCode": 74,
          "Message": "Repository commit is not set"
        },
        {
          "ErrorCode": 73,
          "Message": "Repository URL is not set"
        },
        {
          "ErrorCode": 72,
          "Message": "Repository type is not set"
        },
        {
          "ErrorCode": 61,
          "Message": "Readme is not set"
        },
        {
          "ErrorCode": 51,
          "Message": "Project url is not set"
        },
        {
          "ErrorCode": 22,
          "Message": "The nuspec file use the deprecated licenseUrl metadata"
        },
        {
          "ErrorCode": 31,
          "Message": "The nuspec file use the deprecated iconUrl metadata"
        }
      ]
    }
  }
}

I can tackle those if you want. 🙂

Toreno96 commented 1 year ago

I added --skip-duplicate flag, so publish won't fail after merging this.

@vlnevyhosteny Is that required because we have already published version 2.0.0 (when upgrading to .NET 2.1) and now we're publishing version 2.0.0 again (with release-please-related changes)?

ViolanteCodes commented 1 year ago

@vlnevyhosteny

  • [ ] ⚠️ We have to set up NUGET_APIKEY secret before this PR is going to be merged.

Okay - I'll ping Jake on this, as he usually handles the service keys.

  • I added job for validating NuGET package right before publish. However, I had to skip this job since there are some error during validation:

I can tackle those if you want. 🙂

Yes, please fix. And let us know on Daniels' comment above re:

@vlnevyhosteny Is that required because we have already published version 2.0.0 (when upgrading to .NET 2.1) and now we're publishing version 2.0.0 again (with release-please-related changes)?

vlnevyhosteny commented 1 year ago

I added --skip-duplicate flag, so publish won't fail after merging this.

@vlnevyhosteny Is that required because we have already published version 2.0.0 (when upgrading to .NET 2.1) and now we're publishing version 2.0.0 again (with release-please-related changes)?

@Toreno96 I depends. It's not necessery for the future releases. Unless someone manually trigger publish workflow again. But in this case we are technicaly re-releasing 2.0.0 again so it will fail. There are 3 options:

  1. Keep --skip-duplicate. The workflow won't fail. It's not going to publish to NuGET again, but it will silently pass.
  2. Remove --skip-duplicate so the workflow is going to fail.
  3. Remove --skip-duplicate and increment patch version (2.0.1). So there will be a new release without any impact on users.

As I'm thinking about that, option 3 makes sense the most.

ViolanteCodes commented 1 year ago

As I'm thinking about that, option 3 makes sense the most.

I am fine with this. Releasing a 2.0.1 would also let us test out the SDK push to NuGet and just make sure everything is working properly. Same as before then - we need this commits to not be chore so that they'll actually open a version release.

Toreno96 commented 1 year ago

I agree that the option 3 sounds the most reasonable 👍

vlnevyhosteny commented 1 year ago

@ViolanteCodes @Toreno96 So, all validation errors are fixed in https://github.com/ButterCMS/buttercms-csharp/pull/40/commits/013ed27cd9352dc0d72c8a82b6048f71703d2e16. There will be full NuGET listing like this https://www.nuget.org/packages/Vladss.ButterCMS/2.0.1#readme-body-tab . I also edited https://github.com/ButterCMS/buttercms-csharp/pull/40/commits/eadea9adbaa09bcc42ea9e409e2ebbfed2b6efb2 to invoke 2.0.1 release and removed --skip-duplicate flag. I think we are ready to go when NUGET_APIKEY secret is provided to the repo 🙂

jakelumetta commented 1 year ago

I've added NUGET_APIKEY as a secret to the repo.