Skip to content

Revamp the Nuget publish workflow#3255

Open
ZakFahey wants to merge 1 commit intoPryaxis:general-develfrom
ZakFahey:nuget
Open

Revamp the Nuget publish workflow#3255
ZakFahey wants to merge 1 commit intoPryaxis:general-develfrom
ZakFahey:nuget

Conversation

@ZakFahey
Copy link
Contributor

@ZakFahey ZakFahey commented Mar 7, 2026

  • The workflow now runs every time a release is created, rather than when a commit is pushed into the nuget-release branch.
  • If there's a mismatch between the release tag and the project tag, the release tag will take precedence. If such a version override happens, there will be a workflow warning.
  • It is recommended that for pre-releases, you don't update the version number in the csproj file. This will ensure that UpdateManager.cs continues to work as expected (it does not currently handle version numbers with a prerelease suffix, and otherwise we'd have to add logic to not alert for prerelease updates). But once you have a mainline release, the version number should be updated.

The goal here is to eliminate all human error that would cause the Nuget package to ever not be updated. If there is a GitHub release, the Nuget release will always be there, including for pre-releases.

The utility of the Nuget package is greatly diminished if it isn't consistently up-to-date. I was planning soon to update a bunch of plugins to the latest pre-release, many of which currently reference TShock using the Nuget package. Having the benefits of a package reference at one point, and then being forced to go back to a binary reference later on, and then just continuing to switch back and forth, makes for a less ergonomic updating experience.

Changelog updates: N/A

- The workflow now runs every time a release is created, rather than when a commit is pushed into the nuget-release branch.
- If there's a mismatch between the release tag and the project tag, the release tag will take precedence. If such a version override happens, there will be a workflow warning.
- It is recommended that for pre-releases, you don't update the version number in the csproj file. This will ensure that UpdateManager.cs continues to work as expected (it does not currently handle version numbers with a prerelease suffix, and otherwise we'd have to add logic to not alert for prerelease updates). But once you have a mainline release, the version number should be updated.

The goal here is to eliminate all human error that would cause the Nuget package to ever not be updated. If there is a GitHub release, the Nuget release will always be there, including for pre-releases.
@hakusaro
Copy link
Member

hakusaro commented Mar 7, 2026

I don’t have any intention of changing the nuget release cycle.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 7, 2026

Greptile Summary

This PR replaces the previous nuget-release branch-push trigger with a GitHub release: published event trigger, ensuring the NuGet package is automatically published for every GitHub release (including pre-releases) without any manual branch management.

Key changes:

  • Trigger: Workflow now fires on release: types: [published] instead of a push to the nuget-release branch, eliminating the human-error risk of forgetting to push the branch.
  • Version override from tag: dotnet build is called with -p:PackageVersion=${GITHUB_REF_NAME#v}, so the NuGet package version always matches the release tag. The csproj <Version> (used by AssemblyVersion / UpdateManager.cs) is intentionally left unchanged for pre-releases to avoid confusing the update manager with prerelease suffixes it doesn't understand.
  • Mismatch warning: A new shell step compares the csproj <Version> with the release tag and emits a ::warning:: annotation if they differ, surfacing the discrepancy without blocking the publish.
  • --skip-duplicate on push: Prevents a CI failure if the workflow is re-run for the same tag, making the publish idempotent.
  • TShockAPI.csproj comment: Updated to document the new pre-release versioning policy (don't bump <Version> for pre-releases; only do so for full releases).

Confidence Score: 4/5

  • This PR is safe to merge; the logic is sound and the only issue is a minor unquoted variable that is unlikely to matter in practice.
  • The workflow refactor is well-reasoned and addresses a real reliability gap. The version-from-tag mechanism (-p:PackageVersion) is idiomatic MSBuild usage, the GeneratePackageOnBuild=true setting in the csproj means no separate dotnet pack step is needed, and --skip-duplicate makes publishes idempotent. The only finding is the unquoted ${GITHUB_REF_NAME#v} in the build command, which is a defensive-coding concern rather than a likely runtime failure.
  • .github/workflows/ci-otapi3-nuget.yml line 34 — unquoted variable expansion in build command.

Important Files Changed

Filename Overview
.github/workflows/ci-otapi3-nuget.yml Revamped workflow trigger from branch push to GitHub release published event; added version-mismatch warning step and --skip-duplicate guard; minor unquoted variable expansion in build command.
TShockAPI/TShockAPI.csproj Comment in the version block updated to clarify the pre-release versioning policy; no functional changes to the project file itself.

Sequence Diagram

sequenceDiagram
    participant Dev as Developer
    participant GH as GitHub Release
    participant CI as GitHub Actions (publish job)
    participant NuGet as NuGet Registry

    Dev->>GH: Create & publish release (tag: v5.9.9-beta1)
    GH-->>CI: Trigger: release published
    CI->>CI: Checkout repo (recursive submodules)
    CI->>CI: Setup .NET 9.0
    CI->>CI: Check csproj version vs tag<br/>(warn if mismatch)
    CI->>CI: dotnet restore
    CI->>CI: dotnet build -p:PackageVersion=5.9.9-beta1<br/>(generates TShock.5.9.9-beta1.nupkg)
    CI->>CI: dotnet test --no-build --configuration Release
    CI->>NuGet: dotnet nuget push *.nupkg --skip-duplicate
    NuGet-->>CI: 200 OK (or 409 skipped if duplicate)
Loading

Last reviewed commit: 2b9c779


- name: Build
run: dotnet build TShock.sln --configuration Release --no-restore
run: dotnet build TShock.sln --configuration Release --no-restore -p:PackageVersion=${GITHUB_REF_NAME#v}
Copy link
Contributor

Choose a reason for hiding this comment

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

Unquoted variable expansion in build command

The ${GITHUB_REF_NAME#v} expansion is not quoted. If a release tag ever contains spaces or other special shell characters (e.g., v5.9.9 beta1), the unquoted expansion would result in word splitting — beta1 would be passed as a separate argument to dotnet build, causing the command to fail with an unexpected argument error.

While GitHub release tags with spaces are atypical, quoting the parameter is a defensive best practice:

Suggested change
run: dotnet build TShock.sln --configuration Release --no-restore -p:PackageVersion=${GITHUB_REF_NAME#v}
run: dotnet build TShock.sln --configuration Release --no-restore "-p:PackageVersion=${GITHUB_REF_NAME#v}"

@hakusaro
Copy link
Member

hakusaro commented Mar 7, 2026

For starters, we create pre-releases that are unstable. Second, with the advent of AI LLMs, updating your references should be trivial.

@ZakFahey
Copy link
Contributor Author

ZakFahey commented Mar 7, 2026

Is there any particular reason to avoid having prerelease Nuget packages for TShock prereleases? If it's tagged as a prerelease in Nuget, developers would know and understand that it is unstable already.

Also, plugins that reference TShock would usually be okay to reference a prerelease, because the main reason for doing so is to update against the API breaking changes. Between prerelease and main release I probably wouldn't expect breaking changes at least for package referencing, so getting a head start at updating plugins with a prerelease is often an okay final state even after the main release with all its bugfixes is out.

@hakusaro
Copy link
Member

hakusaro commented Mar 7, 2026

Your changes don't tag anything as a pre-release do they?

I would prefer we discuss this set of practices in-general. If you open a PR without even discussing it, I think it leads to counterproductive "fighting against the current".

  1. I'm not opposed in-principle to nuget pre-releases, but it has never been asked of us.
  2. We don't really have tags that make logical sense. Maybe the tags are fine for nuget? I'm not sure. Pre-release tagging and versioning wasn't standardized.
  3. I prefer to have a manual flow ala what OTAPI does, because it gives us control over what is published versus what is not.

I also consider build staining a general pre-requisite to this. I would like it if every build we release is stamped with the git version as well, so we know exactly what is where. Because we do not have build staining, I am reluctant to release anything that is not an official release to nuget.

@hakusaro
Copy link
Member

hakusaro commented Mar 7, 2026

If you're interested in adding build staining, we could add automatic publishing to nuget for pre-releases afterwards, provided they are tagged as such*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants