Tighten package sources in C# smoketests #1735
Conversation
|
Awesome, looks like this currently catches an issue that will be fixed by #1734. |
Same as clockworklabs/SpacetimeDB#1735 but for this repo.
RReverser
force-pushed
the
ingvar/csharp-smoketest-stricter-nuget-config
branch
from
50f971c to
84201f3
Compare
| <packageSource key="Local SpacetimeDB.BSATN.Runtime"> | ||
| <package pattern="SpacetimeDB.BSATN.Runtime" /> | ||
| </packageSource> | ||
| </packageSourceMapping> |
There was a problem hiding this comment.
FWIW, the previous nuget-fallback behavior was entirely intentional (see the PR description that introduced it: #1503).
tl;dr it was set up so that, when we released breaking changes in this repo, the SDK tests would continue to use a package they were compatible with, rather than breaking for every future SpacetimeDB PR until someone fixed the C# SDK.
There was a problem hiding this comment.
This way, the C# SDK tests only break if you fail to bump the version numbers when you make a breaking change.
I don't feel strongly about keeping it that way (and I believe it's not a required check, so this doesn't "deadlock" us), but that was the original intent.
There was a problem hiding this comment.
This way, the C# SDK tests only break if you fail to bump the version numbers when you make a breaking change.
Problem is, we also have the opposite sometimes - we bumped SpacetimeDB version, made some breaking changes, but C# SDK tests are still passing "successfully" because they are still pointing to old BSATN.* packages.
This is too easy to miss at the moment, so I do think the noisy error would be better in this context as it would allow us to catch issues sooner rather than later.
There was a problem hiding this comment.
Although I can see the arguments for the original behaviour too...
There was a problem hiding this comment.
Sounds good - we should probably add a requirement that people breaking a downstream repo (in this case, the C# SDK) have a fix PR ready to go before merging their breaking change in this repo.
There was a problem hiding this comment.
I'd say let's try this stricter mode to catch breaking behaviour sooner; if it does prove too annoying, we can always revert those changes.
Before this, we couple of times ran into issues where a version in SpacetimeDB template was outdated, or NuGet sources were in incorrect order, and so on, which led to either tests failing with misleading error (2b09c8d being most recent example), or, worse, quietly passing even when code or versions are actually broken. Apparently, NuGet has a package source mapping feature which allows to ensure that specific packages are only pulled from specific custom sources, which is exactly what we want here. It allows us to ensure that SpacetimeDB.* pacakges for smoketests are pulled from our local build folders, and not from nuget.org.
RReverser
force-pushed
the
ingvar/csharp-smoketest-stricter-nuget-config
branch
from
84201f3 to
71e900e
Compare
## Description of Changes Same as clockworklabs/SpacetimeDB#1735 but for this repo. ## API - [ ] This is an API breaking change to the SDK *If the API is breaking, please state below what will break* ## Requires SpacetimeDB PRs *List any PRs here that are required for this SDK change to work*
## Description of Changes Same as #1735 but for this repo. ## API - [ ] This is an API breaking change to the SDK *If the API is breaking, please state below what will break* ## Requires SpacetimeDB PRs *List any PRs here that are required for this SDK change to work*
## Description of Changes Same as clockworklabs/SpacetimeDB#1735 but for this repo. ## API - [ ] This is an API breaking change to the SDK *If the API is breaking, please state below what will break* ## Requires SpacetimeDB PRs *List any PRs here that are required for this SDK change to work*
Description of Changes
Before this, we couple of times ran into issues where a version in SpacetimeDB template was outdated, or NuGet sources were in incorrect order, and so on, which led to either tests failing with misleading error (2b09c8d being most recent example), or, worse, quietly passing even when code or versions are actually broken.
Apparently, NuGet has a package source mapping feature which allows to ensure that specific packages are only pulled from specific custom sources, which is exactly what we want here. It allows us to ensure that SpacetimeDB.* pacakges for smoketests are pulled from our local build folders, and not from nuget.org.
API and ABI breaking changes
If this is an API or ABI breaking change, please apply the
corresponding GitHub label.
Expected complexity level and risk
How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.
This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.
If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.
Testing
Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!