Allow doing testmerges from different repositories for forks. - #2431
Allow doing testmerges from different repositories for forks.#2431misleadingname wants to merge 5 commits into
Conversation
|
Thank you for contributing to tgstation-server! The CI Pipeline workflow requires repository secrets and will not run without approval. Maintainers can add the |
Cyberboss
left a comment
There was a problem hiding this comment.
- Needs minor version bumps to core, api, api library, and client in
build/Version.props. - Needs a changelog entry for
REST API,Nuget: Api, andNuget: Client. Also remove that "Categories" entry you have now. - Needs down migration selection based on previous server version. (The tests are going to complain about this).
- Fork https://github.com/Cyberboss/common_core and open a pull request on that fork. Then add tests to
RepositoryTest.TestMergeTests. - This is going to need a new
RepositoryRightsentry (i.e.ForkTestMerges) that needs checking inRepositoryController.cswhere test merges are added ifTestMergeParameters.SourceRepositoryis set.
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public (string owner, string name) GetRepositoryOwnerAndName(TestMergeParameters parameters) |
There was a problem hiding this comment.
PascalCase for named tuples please
| var numberMatch = testRevInfo.PrimaryTestMerge.Number == testTestMerge.Number; | ||
| if (!numberMatch) | ||
| return false; | ||
| var numberMatch = testRevInfo.PrimaryTestMerge.Number == testTestMerge.Number; |
| @@ -0,0 +1,29 @@ | |||
| using Microsoft.EntityFrameworkCore.Migrations; | |||
There was a problem hiding this comment.
Format with tabs not spaces on all the migration files.
There was a problem hiding this comment.
The log message should be changed if it's from a separate remote
There was a problem hiding this comment.
Both these branch names should be changed if a separate remote is in use. Which means IGitRemoteFeatures.TestMergeLocalBranchNameFormatter probably needs changing to a function
| logger.LogTrace("Fetching refspec {refSpec}...", refSpec); | ||
|
|
||
| var (owner, name) = gitRemoteFeatures.GetRepositoryOwnerAndName(testMergeParameters); | ||
| var remote = libGitRepo.Network.Remotes.First(); |
There was a problem hiding this comment.
A lot of repository code assumes there is ever only one remote and since we don't know if it's deterministically ordered or not we need to check every place it's used and change it to refer to origin by name.
This one for example is no longer valid after the addition of an extra remote.
There was a problem hiding this comment.
IGitRemoteFeatures.TestMergeRefSpecFormatter also needs to become a function of source repository
| public int Number { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// The source repository of the test merge. If not specified, the repository the server is configured to use will be used. |
There was a problem hiding this comment.
| /// The source repository of the test merge. If not specified, the repository the server is configured to use will be used. | |
| /// The source repository slug of the test merge. If not specified, the repository the server is configured to use will be used. |
| .MapMySqlTextField<TestMerge>(x => x.BodyAtMerge) | ||
| .MapMySqlTextField<TestMerge>(x => x.Comment) | ||
| .MapMySqlTextField<TestMerge>(x => x.TargetCommitSha) | ||
| .MapMySqlTextField<TestMerge>(x => x.SourceRepository) |
There was a problem hiding this comment.
This list is for prior migrations and shouldn't be necessary here. Note that removing it will require you to regenerate the migrations.
| EventType.RepoAddTestMerge, | ||
| new List<string?> | ||
| { | ||
| testMergeParameters.Number.ToString(CultureInfo.InvariantCulture), |
There was a problem hiding this comment.
An additional parameter should be added to EventType.RepoAddTestMerge with the source repository if it's present. Documentation on the enum will also need updating
|
I really hope I did the changelog thing properly, also it's my first time actually doing a PR like that; so apologies if I did something wrong! |
Cyberboss
left a comment
There was a problem hiding this comment.
Sorry, was pretty busy this weekend
| () => | ||
| { | ||
| var remote = libGitRepo.Network.Remotes.First(); | ||
| var remote = libGitRepo.Network.Remotes["origin"]; |
There was a problem hiding this comment.
Make this a class const string please.
There was a problem hiding this comment.
it's actually unused so I'll get rid of the instance of this one, you probably meant all of those? Also I don't really get what you mean by making it a const string; as in make the remotes a string and not Remotes?
There was a problem hiding this comment.
like
class Repository {
const string DefaultRemoteName = "origin";
...and then reference that instead of copying "origin" everywhere.
| | RepositoryRights.UpdateBranch | ||
| | RepositoryRights.ChangeSubmoduleUpdate)] | ||
| | RepositoryRights.ChangeSubmoduleUpdate | ||
| | RepositoryRights.OffRepoTestMerges)] |
There was a problem hiding this comment.
I'd undo this change, OffRepoTestMerges by itself shouldn't grant proper access to this method. The MergePullRequest handles that.
|
Excuse the big delay, my primary computer broke and I had to wait for parts to arrive. |
|
bump |
|
Sorry works busy as all hell. Feel free to ping me in Discord after 5PM EST
to remind me
…On Mon, Mar 23, 2026, 7:29 a.m. misname ***@***.***> wrote:
*misleadingname* left a comment (tgstation/tgstation-server#2431)
<#2431 (comment)>
bump
—
Reply to this email directly, view it on GitHub
<#2431?email_source=notifications&email_token=AB6LA6SXQURV3Y63Y7WE3UD4SEN2HA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMJQHE4TENJTHA4KM4TFMFZW63VGMFZXG2LHN2SWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4109925388>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB6LA6SANFYJLQP6XP43MW34SEN2HAVCNFSM6AAAAACVEGNMFKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DCMBZHEZDKMZYHA>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
|
Sorry, cleared the tests |
Co-authored-by: Jordan Dominion <jordanhcbrown+github@gmail.com>
🆑 REST API
Added support for performing test merges from different repositories, enabling fork workflows that track upstream PRs.
/:cl:
🆑 Nuget: API
Exposed support for cross-repository test merges.
/:cl:
🆑 Nuget: Client
Added client support for initiating cross-repository test merges.
/:cl:
Really useful for forks that are really close to their upstream, allows to pull in and use test merges from their upstream PRs that would either need to be manually mirrored or merged in. Mostly a huge QoL change.