-
Notifications
You must be signed in to change notification settings - Fork 110
Add SurrealDB hosting/client integration #251
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
Conversation
@Odonno thanks for the contribution. For runnig CI Actions, Please fix the confilict with the main brach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A good start and I can see how it'll fit into the Toolkit overall. As @Alirexaa mentioned, there's some merge conflicts that will block running a CI job on this.
I've left some comments/changes throughout the PR too.
/examples/surrealdb/_ @Odonno | ||
/src/CommunityToolkit.Aspire.Hosting.SurrealDb/_ @Odonno | ||
/tests/CommunityToolkit.Aspire.Hosting.SurrealDb.Tests/_ @Odonno | ||
/src/CommunityToolkit.Aspire.SurrealDb/_ @Odonno | ||
/tests/CommunityToolkit.Aspire.SurrealDb.Tests/_ @Odonno |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the CODEOWNERS to be valid you would need to join the maintainer group - is that something you're interested in (I'm guessing so since you put yourself down as the owner 😉)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be good now. Can you reopen the PR @aaronpowell ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaronpowell Are you there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unable to reopen the PR, looks like the branch has had a force push done since it was closed so the PR isn't able to reopen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I opened a new PR #365
...ire.Hosting.SurrealDb.ApiService/CommunityToolkit.Aspire.Hosting.SurrealDb.ApiService.csproj
Show resolved
Hide resolved
...it.Aspire.Hosting.SurrealDb.AppHost/CommunityToolkit.Aspire.Hosting.SurrealDb.AppHost.csproj
Show resolved
Hide resolved
namespace CommunityToolkit.Aspire.Hosting.SurrealDb.Tests; | ||
|
||
[RequiresDocker] | ||
public class SurrealDbFunctionalTests(ITestOutputHelper testOutputHelper) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm cautious on the maintainability of these tests, as we've had some problems in the past with this style in addition to the ones that use the examples app host project.
Once we're able to run CI on them, let's evaluate properly, but also consider what these tests are testing that the functionality the Community Toolkit provides vs what is .NET Aspire. For example, if the volume persistence fails, is that a Toolkit bug or Aspire bug? Assuming all our tests cover setting the right annotations isn't the Toolkit doing the right stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't really tell. But at least it can confirm that the database integration works as expected for those scenarii.
|
||
namespace CommunityToolkit.Aspire.Hosting.SurrealDb.Tests; | ||
|
||
public class SurrealDbPublicApiTests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get some tests in here that assert all the right annotations are setup across the container, the mounts, args, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an example? I suppose Verify would be useful for this kind of tests?
Closes #<ISSUE_NUMBER>
Project Additions and Updates
Dependency Updates
SurrealDb.Net
as a dependency of the SurrealDB client integrationBogus
to generate fake data, to be used for testing purposesSurrealDb.MinimalApis.Extensions
to provide extension methods for Minimal APIs. Used to create API endpoints for theexample
API service project with the minimum of code.Code Ownership
CODEOWNERS
file to include new SurrealDB projects and assigned ownership to @OdonnoPR Checklist
Other information
Unable to run tests that RequiresDocker as of now. For some reasons, the containers are never created. Maybe this is the reason. I am using Rancher Desktop. For info, those tests worked in the previous PR I mentioned.