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

CI checks for the Unity SDK (and eventually other SDKs) are not exactly correct #214

Open
cloutiertyler opened this issue Jan 14, 2025 · 3 comments

Comments

@cloutiertyler
Copy link
Collaborator

cloutiertyler commented Jan 14, 2025

Currently in the https://github.com/clockworklabs/com.clockworklabs.spacetimedbsdk repo we have CI checks that run for every PR and then also on every commit in master.

Previously, our .NET Build CI tests used to just checkout the latest master of SpacetimeDB, for the purpose of building the C# libraries that are shared by the SDK and the module library, and then build the SDK against that.

The issue with this is that if the SpacetimeDB version has changed (or a breaking changed has been introduced) since the version that the Unity SDK is dependent on, then the CI check will fail. (e.g. it will try to depend against 0.8.0 but only 0.9.0 is available since SpacetimeDB master is 0.9.0).

@jdetter tried to resolve this issue for the Unity Test Suite, by allowing the PR author to specify a branch/ref/tag against which the Unity CI would run. This solves the failing tests for the SDK, however, it doesn't solve the problem in general, because when CI runs again after the PR merges, there is no longer a specified branch against which to run and CI must assume master.

DoD

It's not possible for tests to fail because of a version change to the SpacetimeDB repo that the SDK does not depend on.

@bfops
Copy link
Collaborator

bfops commented Jan 29, 2025

FWIW the intention is that someone submitting a breaking change to SpacetimeDB is also responsible for fixing that change in downstream repositories that depend on SpacetimeDB. If we leave that work until an arbitrarily later time, we run the risk of PRs merging into the Unity SDK that will complicate the breaking change. The best "hygiene" is to keep the repositories in close sync wherever possible.

@bfops
Copy link
Collaborator

bfops commented Jan 29, 2025

For what it's worth, an easy-ish fix is to just allow the CI tests to pull packages from NuGet, rather than only allowing the ones from SpacetimeDB master. We used to allow this. There was a long discussion about the decision to change this somewhere, but I can't find it.

I'm very open to going back to that version of the world, if we agree that we'd be decoupling the Unity SDK from SpacetimeDB core, to more of a conventional inter-repository flow where the downstream repos are just periodically updated in one big update PR.

Either way it should matter less post 1.0.

@bfops
Copy link
Collaborator

bfops commented Feb 7, 2025

Ah, there's another failure case here: the test suite now checks out the specified SpacetimeDB branch and regenerates the DLLs, but those DLLs only apply to the C# project (via a NuGet override). The regenerated DLLs do not apply to the Unity project, so the Unity project ends up failing regardless.

This is another case where it's confusing that we have two very different paths to testing when the Unity SDK is 99% of the use case here.

We've also opened up a bad path where PRs can merge because they succeed when pointed at a branch of SpacetimeDB, even though they may fail when pointed at any master commit. It seems like they should have to point at a master commit (and pass) in order to merge.

I would suggest that we do less dancing here, rather than trying to do the perfect dance. However, it again hopefully will matter less after 1.0.

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

No branches or pull requests

2 participants