Skip to content

Bump NATS.Jwt, add net10, switch to orbit test server#19

Merged
mtmk merged 3 commits into
mainfrom
update-deps-net10-orbit
Apr 22, 2026
Merged

Bump NATS.Jwt, add net10, switch to orbit test server#19
mtmk merged 3 commits into
mainfrom
update-deps-net10-orbit

Conversation

@mtmk
Copy link
Copy Markdown
Collaborator

@mtmk mtmk commented Apr 21, 2026

Bumps NATS.Jwt to 1.0.0, adds net10.0 to all projects, migrates the solution file to .slnx, and swaps the locally-vendored NatsServerProcess for the Synadia.Orbit.Testing.NatsServerProcessManager package.

Connect_with_jwt passes withJs=false because orbit's default JetStream flag (-js -sd ...) prevents the operator-mode server from writing its ports file within the startup retry window.

- NATS.Jwt 1.0.0-preview.16 -> 1.0.0
- Add net10.0 to library, examples, tests, compat targets
- Migrate callout.net.sln to .slnx
- Replace local NatsServerProcess with Synadia.Orbit.Testing.NatsServerProcessManager 1.0.0-preview.4
- Pass withJs=false for Connect_with_jwt since orbit's default JetStream flag breaks operator-mode server startup
@synadia-claude-reviewer
Copy link
Copy Markdown

synadia-claude-reviewer Bot commented Apr 21, 2026

Claude finished @mtmk's task in 1m 52s —— View job


PR Review

  • Gather context (CLAUDE.md, PR diff, changed files)
  • Review dependency and project file changes
  • Review test infrastructure migration (NatsServerProcess → Orbit)
  • Review AuthServiceTest.cs changes
  • Post inline comments and summary

Summary

Clean housekeeping PR. The NATS.Jwt bump to stable, .slnx migration, and replacement of the vendored NatsServerProcess.cs with Synadia.Orbit.Testing.NatsServerProcessManager all look correct. The withJs: false workaround in Connect_with_jwt is consistent with the PR description (orbit's JetStream flags interfere with operator-mode ports file writing) and the other two tests correctly omit it since they don't use operator-mode configs.

One real issue found:

Directory.Build.props line 40net9.0 was replaced by net10.0 in the AOT/trimming condition instead of adding net10.0 alongside net9.0. The library still ships a net9.0 TFM (it's in TargetFrameworks), so net9.0 consumers will silently lose IsTrimmable=true/isAotCompatible=true vs. today. Inline suggestion posted.

One observation:

The test project targets net8.0;net10.0, skipping net9.0, so the AOT regression above won't be caught by CI. May be an intentional tradeoff to keep the matrix lean, but worth being aware of.

Minor note:

Synadia.Orbit.Testing.NatsServerProcessManager is a preview package (1.0.0-preview.4). That's fine for a test-only dependency from the same org, just flagging it for visibility. Fix this →

Comment thread Directory.Build.props Outdated
Comment thread tests/Synadia.AuthCallout.Tests/Synadia.AuthCallout.Tests.csproj Outdated
- Directory.Build.props: restore net9.0 in IsTrimmable/isAotCompatible condition
- test.yml: update X_COMPAT_EXE to bin/Debug/net10.0/compat to match compat TFM bump
@mtmk mtmk closed this Apr 22, 2026
@mtmk mtmk reopened this Apr 22, 2026
@synadia-claude-reviewer

This comment was marked as resolved.

Comment thread .github/workflows/test.yml
Comment thread tests/Synadia.AuthCallout.Tests/Synadia.AuthCallout.Tests.csproj Outdated
- Tests.csproj: add net9.0 so the TFM the library ships is actually exercised
- test.yml: add 10.x to setup-dotnet since we build net10.0 binaries
@mtmk mtmk closed this Apr 22, 2026
@mtmk mtmk reopened this Apr 22, 2026
@synadia-claude-reviewer
Copy link
Copy Markdown

synadia-claude-reviewer Bot commented Apr 22, 2026

Claude finished @mtmk's task in 2m 44s —— View job


PR Review

  • Gather context (CLAUDE.md, PR diff, changed files)
  • Review dependency and project file changes
  • Review test infrastructure migration (NatsServerProcess → Orbit)
  • Review AuthServiceTest.cs changes
  • Review CI workflow changes
  • Post inline comments and summary

Summary

This PR is in good shape. The three issues flagged in earlier review passes have all been resolved in subsequent commits:

Earlier concern Resolution
AOT condition dropped net9.0 from Directory.Build.props Fixed in commit 962c7c2 — condition now correctly reads net8.0 OR net9.0 OR net10.0
net9.0 missing from test matrix Fixed in commit c6563d4 — test project now targets net8.0;net9.0;net10.0
CI setup-dotnet missing 10.x Fixed in commit c6563d4 — all three SDK versions installed explicitly

Changes reviewed

Directory.Build.props — Cleanup is correct. The removed netstandard2.1 conditions were dead code (netstandard2.1 was never in TargetFrameworks). The removed net6.0 OR net8.0 condition for System.Text.Json is also safe: net6.0 was never targeted, and net8.0+ includes System.Text.Json in the BCL.

AuthServiceTest.cswithJs: false only on Connect_with_jwt is consistent with the PR description. The other two tests use non-operator-mode configs and don't need it.

Synadia.AuthCallout.csprojNATS.Jwt promoted from 1.0.0-preview.16 to 1.0.0 stable. TFM list correctly extended with net10.0.

.github/workflows/test.ymlX_COMPAT_EXE path updated to net10.0; all three SDKs explicitly installed. Correct.

Minor observations

  • Synadia.Orbit.Testing.NatsServerProcessManager is 1.0.0-preview.4. Acceptable for a test-only dep from the same org.
  • NATS.Client.Services remains at 2.5.12 (unchanged by this PR, not a concern here).

No blocking issues. The PR looks ready to merge.

@mtmk mtmk merged commit 040354f into main Apr 22, 2026
10 checks passed
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.

1 participant