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

CrdtMerge API web service #1136

Merged
merged 50 commits into from
Oct 25, 2024
Merged

CrdtMerge API web service #1136

merged 50 commits into from
Oct 25, 2024

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Oct 21, 2024

Fixes #1091.

Work done:

  • Sync CRDT and FW projects
  • Do Send/Receive with Lexbox after sync
  • Sync CRDT and FW projects again if S/R pulled down any changes from others
  • Create API endpoint to trigger all of this

Still remaining:

  • Create k8s deployment Put off for later PR
  • Write tests to exercise it by sending API requests and verifying correct results happen Put off for later PR

To test

  • Edit appsettings.Development.json and choose a different folder than ../../hgweb/repos if you want, otherwise the test will put files into lexbox/hgweb/repos
  • Run dotnet run inside the CrdtMerge folder
  • Send a POST request to http://localhost:5275/sync?projectCode=sena-3
  • Should take a minute or so to complete, watch the dotnet run logs while you wait
  • Verify that the JSON result has the right number of changes (should be 1462, one per entry)
  • Verify that the CRDT SQLite files showed up in lexbox/hgweb/repos, or wherever you put them

@rmunn rmunn self-assigned this Oct 21, 2024
Copy link

github-actions bot commented Oct 21, 2024

C# Unit Tests

75 tests   75 ✅  5s ⏱️
13 suites   0 💤
 1 files     0 ❌

Results for commit 4e72940.

♻️ This comment has been updated with latest results.

backend/CrdtMerge/Program.cs Fixed Show fixed Hide fixed
backend/CrdtMerge/Program.cs Fixed Show fixed Hide fixed
backend/CrdtMerge/Program.cs Fixed Show fixed Hide fixed
@rmunn
Copy link
Contributor Author

rmunn commented Oct 22, 2024

Running into sillsdev/flexbridge#409 while working on this. There's no reason built into FLExBridge why FixFwData.exe has to have an .exe filename. I created this problem for myself because I didn't fully understand how FlexBridge worked at the time, then spent a lot of effort working around the problem I myself created. But really, I should just relax the requirements of where FixFwData.exe needs to be located, and then everything will work as it should.

@rmunn
Copy link
Contributor Author

rmunn commented Oct 24, 2024

About to rebase on top of current develop to fix LexBox.sln merge conflicts, then I'll take this out of draft status since it's ready for initial review. Not ready to push yet since I still need to create the k8s deployment (and figure out where it should store CRDT projects), but I just ran this successfully on my dev machine, and synced sena-3 from my local LexBox into a CRDT project.

rmunn added 23 commits October 24, 2024 10:11
This is basically the `dotnet new` template for an ASP.NET server, with
the example API removed and replaced with one that takes a project code.
This is copied nearly as-is, but with an ASP.NET Core API wrapped around
it instead of a System.CommandLine set of arguments and options. Next
step would be to move the implementing classes into a CrdtMerge.Core
library so that FwLiteProjectSync can reference them and we DRY
ourselves out again.
This requires removing `<SelfContained>true</SelfContained>` from the
FwLiteProjectSync.csproj file, but if in the future we do decide to
deploy FwLiteProjectSync as a self-contained exe, we can just pass the
appropriate compiler flag on the command line. Meanwhile, this avoids
needing to create a CrdtSync.Core library while still sharing code.
These are implicit in the Microsoft.NET.Sdk.Web SDK, so we don't need
to redeclare them here.
Needed for VS Code C# extension to give proper Intellisense
This is a little finicky, because it requires running from the directory
where FixFwData.exe is located. At some point I'll fix that too-strict
requirement in LfMergeBridge and then the extra build steps aorund
FixFwData.exe can go away.
This will allow us to create a Services class that's dependency injected
and gets thigns like HgConfig, while the functions in the Helpers class
take all their required config in their params and remain static.
Next step is to clean up slightly-messy code and separate properly
into different files.
No more lambda that just calls the implementation; just pass the
implementation method in directly as that's cleaner.
Compiles but doesn't run, as LcmCrdtKernel.ConfigureDbOptions complains
that Project is null. But we have the basic skeleton of the tool now.
Now LfMergeBridge can find FixFwData with and without the .exe extension
We need to obtain two services just-in-time: CurrentProjectService, and
IMiniLcmApi. Both of these need projectsService.SetProjectScope to have
been called before they get initialized.
@rmunn rmunn force-pushed the feat/crdt-merge-api branch from 7673487 to b1876ea Compare October 24, 2024 03:15
@rmunn rmunn marked this pull request as ready for review October 24, 2024 03:16
@rmunn rmunn requested a review from hahn-kev October 24, 2024 08:34
rmunn added 8 commits October 24, 2024 15:42
CRDT handles conflicts better than Send/Receive. So first we do a
Send/Receive (or clone if the .fwdata file doesn't exist yet), then we
sync the .fwdata and CRDT, and then we do another Send/Receive of the
sync results in order to push them to Lexbox.
Also rename SRConfig to CrdtMergeConfig since it's not really about
configuring Send/Receive any more, it's about configuring the whole
CrdtMerge application.
The template creates a `/weatherforecast` URL. We deleted it from our
code, but it was still lingering in a couple of places. Gone now.
Now that we're not making it an exe, it's not allowed to use top-level
statements, so we need to move those into a real Program class with a
static Main method.
@rmunn
Copy link
Contributor Author

rmunn commented Oct 24, 2024

Now that this PR is ready, I should work on #1155 because I realized partway through that it's going to be necessary once we're doing S/R of real projects.

@rmunn
Copy link
Contributor Author

rmunn commented Oct 24, 2024

A bunch of FwLite tests are now failing because of the changes we made to how projects are located. Will look at that tomorrow.

@hahn-kev hahn-kev changed the title CrdtMerge API container CrdtMerge API web service Oct 25, 2024
hahn-kev
hahn-kev previously approved these changes Oct 25, 2024
backend/CrdtMerge/SendReceiveHelpers.cs Outdated Show resolved Hide resolved
@rmunn rmunn merged commit 4b27266 into develop Oct 25, 2024
13 checks passed
@rmunn rmunn deleted the feat/crdt-merge-api branch October 25, 2024 09:16
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.

Consider design for CrdtMerge based on LfMerge
2 participants