Skip to content

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

Closed
wants to merge 5 commits into from

Conversation

Odonno
Copy link
Contributor

@Odonno Odonno commented Nov 15, 2024

Closes #<ISSUE_NUMBER>

Project Additions and Updates

  • Added new SurrealDB projects to the solution, including Aspire.CommunityToolkit.Hosting.SurrealDb, Aspire.CommunityToolkit.SurrealDb, and their respective test and example projects

Dependency Updates

  • Added new dependencies:
    • SurrealDb.Net as a dependency of the SurrealDB client integration
    • Bogus to generate fake data, to be used for testing purposes
    • SurrealDb.MinimalApis.Extensions to provide extension methods for Minimal APIs. Used to create API endpoints for the example API service project with the minimum of code.

Code Ownership

  • Updated the CODEOWNERS file to include new SurrealDB projects and assigned ownership to @Odonno

PR Checklist

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • PR doesn't include merge commits (always rebase on top of our main, if needed)
  • New integration
    • Docs are written
    • Added description of major feature to project description for NuGet package (4000 total character limit, so don't push entire description over that)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Contains NO breaking changes
  • Every new API (including internal ones) has full XML docs
  • Code follows all style conventions

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.

@Odonno Odonno changed the title Feat/surrealdb Add SurrealDB hosting/client integration Nov 15, 2024
@Alirexaa
Copy link
Member

@Odonno thanks for the contribution. For runnig CI Actions, Please fix the confilict with the main brach.

Copy link
Member

@aaronpowell aaronpowell left a 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.

Comment on lines +41 to +45
/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
Copy link
Member

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 😉)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not.

Copy link
Contributor Author

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaronpowell Are you there?

Copy link
Member

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

Copy link
Contributor Author

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

namespace CommunityToolkit.Aspire.Hosting.SurrealDb.Tests;

[RequiresDocker]
public class SurrealDbFunctionalTests(ITestOutputHelper testOutputHelper)
Copy link
Member

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?

Copy link
Contributor Author

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
Copy link
Member

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.

Copy link
Contributor Author

@Odonno Odonno Dec 10, 2024

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?

@github-actions github-actions bot added the Stale label Nov 26, 2024
@github-actions github-actions bot closed this Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants