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.

Sign up for GitHub

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

Jump to bottom

WIP: Add Meilisearch integration #66

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

WIP: Add Meilisearch integration #66

wants to merge 11 commits into from
+2,349 −45

Conversation

Alirexaa
Copy link
Member

@Alirexaa Alirexaa commented Oct 4, 2024

Closes #65

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

@Alirexaa Alirexaa added the integration A new .NET Aspire integration label Oct 4, 2024
@Alirexaa Alirexaa self-assigned this Oct 4, 2024
aaronpowell
aaronpowell reviewed Oct 7, 2024
View reviewed changes
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.

Reviewed the WIP as it stands and my only concern is that I'm not sure if we should be shipping the config schema stuff as part of the Community Toolkit packages.

...pire.CommunityToolkit.Hosting.Meilisearch/Aspire.CommunityToolkit.Hosting.Meilisearch.csproj Outdated Show resolved Hide resolved
...pire.CommunityToolkit.Hosting.Meilisearch/Aspire.CommunityToolkit.Hosting.Meilisearch.csproj Outdated Show resolved Hide resolved
src/Aspire.CommunityToolkit.Meilisearch/Aspire.CommunityToolkit.Meilisearch.csproj Outdated Show resolved Hide resolved
src/Aspire.CommunityToolkit.Meilisearch/AssemblyInfo.cs Outdated Show resolved Hide resolved
@Alirexaa Alirexaa mentioned this pull request Oct 17, 2024
Merged
10 tasks
This was referenced Oct 17, 2024
Open
Open
Alirexaa
Alirexaa commented Oct 17, 2024
View reviewed changes
tests/Aspire.CommunityToolkit.Testing/ConformanceTests.cs
@@ -85,18 +86,18 @@ protected virtual void DisableRetries(TOptions options) { }
/// </summary>
protected abstract void SetMetrics(TOptions options, bool enabled);

[ConditionalFact]
[SkippableFact]
Copy link
Member Author

Choose a reason for hiding this comment

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

@aaronpowell, ConditionalFact and ConditionalTheory don't work.
I tried to fix it but did not. I used Xunit.SkippableFact packackge instead.

Can you check why these attributes don't work and if possible fix it?
Otherwise, we can continue using Xunit.SkippableFact package.

@Alirexaa Alirexaa deployed to github-packages October 17, 2024 22:17 — with GitHub Actions Active
aaronpowell
aaronpowell requested changes Oct 17, 2024
View reviewed changes
Directory.Build.props
@@ -13,9 +13,11 @@
<AspireVersion>8.2.0</AspireVersion>
<AspNetCoreVersion>8.0.7</AspNetCoreVersion>
<OpenTelemetryVersion>1.9.0</OpenTelemetryVersion>

<TestcontainersVersion>3.10.0</TestcontainersVersion>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<TestcontainersVersion>3.10.0</TestcontainersVersion>
<TestContainersVersion>3.10.0</TestContainersVersion>

Directory.Packages.props
<!-- Build dependencies -->
<PackageVersion Include="Microsoft.CodeAnalysis.PublicApiAnalyzers" Version="3.3.4" />
<!-- Testcontainers packages -->
<PackageVersion Include="Testcontainers" Version="$(TestcontainersVersion)" />
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<PackageVersion Include="Testcontainers" Version="$(TestcontainersVersion)" />
<PackageVersion Include="Testcontainers" Version="$(TestContainersVersion)" />

...pire.CommunityToolkit.Hosting.Meilisearch/Aspire.CommunityToolkit.Hosting.Meilisearch.csproj

<ItemGroup>
<Compile Include="..\VolumeNameGenerator.cs" Link="Utils\VolumeNameGenerator.cs" />
<!--<Compile Include="$(ComponentsDir)Aspire.Meilisearch\MeilisearchHealthCheck.cs" Link="MeilisearchHealthCheck.cs"></Compile>-->
Copy link
Member

Choose a reason for hiding this comment

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

Is this blocked until aspire-9?

src/Aspire.CommunityToolkit.Meilisearch/Aspire.CommunityToolkit.Meilisearch.csproj
</ItemGroup>

<ItemGroup>
<Compile Include="..\HealthChecksExtensions.cs" Link="Utils\HealthChecksExtensions.cs" />
Copy link
Member

Choose a reason for hiding this comment

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

Should we use RepoRoot here?

src/PasswordGenerator.cs
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 pop the "Copied from aspire " in here

tests/Aspire.CommunityToolkit.Hosting.Meilisearch/AppHostTests.cs
Comment on lines +11 to +14
[Theory]
[InlineData("meilisearch")]
public async Task ResourceStartsAndRespondsOk(string resourceName)
{
Copy link
Member

Choose a reason for hiding this comment

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

Only need to use a Fact here as there's a single resource we're testing against.

Suggested change
[Theory]
[InlineData("meilisearch")]
public async Task ResourceStartsAndRespondsOk(string resourceName)
{
[Fact]
public async Task ResourceStartsAndRespondsOk()
{
var resourceName = "meilisearch";

tests/Aspire.CommunityToolkit.Hosting.Meilisearch/MeilisearchFunctionalTests.cs
new() { Id = "6", Title = "Philadelphia", Genres = ["Drama"] }
];
[Fact]
[RequiresDocker]
Copy link
Member

Choose a reason for hiding this comment

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

Can probably put this on the class rather than the methods.

tests/Aspire.CommunityToolkit.Meilisearch.Tests/AspireMeilisearchClientExtensionsTest.cs
Comment on lines +20 to +23
public AspireMeilisearchClientExtensionsTest(MeilisearchContainerFixture containerFixture)
{
_containerFixture = containerFixture;
}
Copy link
Member

Choose a reason for hiding this comment

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

Use a primary constructor for consistency

tests/Aspire.CommunityToolkit.Testing/Aspire.CommunityToolkit.Testing.csproj
@@ -5,8 +5,13 @@
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
<IsTestProject>false</IsTestProject>
<IsAspireHost>true</IsAspireHost>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to make the test project an Aspire host?

tests/Aspire.CommunityToolkit.Testing/TestDistributedApplicationBuilder.cs
Copy link
Member

Choose a reason for hiding this comment

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

How is this different to the classes exposed by Aspire.Hosting.Testing for creating the DistributedApplication for a test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaronpowell aaronpowell aaronpowell requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@Alirexaa Alirexaa

Labels
integration A new .NET Aspire integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Meilisearch integration
2 participants
@Alirexaa @aaronpowell