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

Add EventStore Hosting and Client integrations #277

Merged
merged 30 commits into from
Nov 29, 2024

Conversation

fredimachado
Copy link
Contributor

@fredimachado fredimachado commented Nov 21, 2024

Closes #236

This pull request includes several updates to the Aspire CommunityToolkit including new projects for EventStore Hosting and Client integrations, a simple "bank account" example project, tests and updates to dependencies.

Project Additions and Updates:

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

Dependency Updates:

Added new dependencies in Directory.Packages.props, including AspNetCore.HealthChecks.EventStore.gRPC, EventStore.Client.Extensions.OpenTelemetry, and EventStore.Client.Grpc.Streams. (Directory.Packages.props) [1]

Code Ownership:

Updated the CODEOWNERS file to include new EventStore projects and assigned ownership to @fredimachado. (CODEOWNERS)

New Example Project:

Added a simple "bank account" example project for EventStore service. (examples/eventstore/Aspire.CommunityToolkit.Hosting.EventStore.ApiService) [1]

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

Docs PR: dotnet/docs-aspire#2143

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.

I don't see tests for the client integration, only the hosting, I assume that they are still to come

@fredimachado
Copy link
Contributor Author

I don't see tests for the client integration, only the hosting, I assume that they are still to come

Addressed in this commit: 8fc728b

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.

Looks really good @fredimachado

I've kicked off a build, once it's clean we can look to merge it in.

You should also have an invite to the contributor team (which will sort out the CODEOWNERS)

Copy link
Member

@Alirexaa Alirexaa left a comment

Choose a reason for hiding this comment

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

Besides of suggested change totally LGTM.

Copy link
Member

@Alirexaa Alirexaa left a comment

Choose a reason for hiding this comment

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

I reviewed tests and did not see tests around persistence and health check functionality.

Could you add these tests?
there are some examples at
https://github.com/CommunityToolkit/Aspire/blob/main/tests/CommunityToolkit.Aspire.Hosting.Meilisearch/MeilisearchFunctionalTests.cs

@fredimachado
Copy link
Contributor Author

I reviewed tests and did not see tests around persistence and health check functionality.

Could you add these tests? there are some examples at https://github.com/CommunityToolkit/Aspire/blob/main/tests/CommunityToolkit.Aspire.Hosting.Meilisearch/MeilisearchFunctionalTests.cs

I thought AppHostTests would also count as persistence tests, since it uses the example API to append and then read events from EventStore.

@Alirexaa
Copy link
Member

I reviewed tests and did not see tests around persistence and health check functionality.
Could you add these tests? there are some examples at https://github.com/CommunityToolkit/Aspire/blob/main/tests/CommunityToolkit.Aspire.Hosting.Meilisearch/MeilisearchFunctionalTests.cs

I thought AppHostTests would also count as persistence tests, since it uses the example API to append and then read events from EventStore.

AppHost does not stop across tests. So it's not enough for all cases.

@fredimachado
Copy link
Contributor Author

I reviewed tests and did not see tests around persistence and health check functionality.
Could you add these tests? there are some examples at https://github.com/CommunityToolkit/Aspire/blob/main/tests/CommunityToolkit.Aspire.Hosting.Meilisearch/MeilisearchFunctionalTests.cs

I thought AppHostTests would also count as persistence tests, since it uses the example API to append and then read events from EventStore.

AppHost does not stop across tests. So it's not enough for all cases.

All good. I added the functional tests based on the Meilisearch ones. d78ff8f

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.

I'm happy with how it's looking with the extra tests, but I'll defer to @Alirexaa for final say

@fredimachado fredimachado force-pushed the eventstore-integration branch from d78ff8f to 2daf3c8 Compare November 25, 2024 11:17
@fredimachado
Copy link
Contributor Author

@Alirexaa I removed the Log volume/mount since it's probably not going to be used often, and we can always use WithVolume extension method in the ResourceBuilder to manually add the log volume if necessary.

@fredimachado
Copy link
Contributor Author

@aaronpowell I noticed the SWA example test is stuck during the build's Test step.
I noticed some packages have vulnerabilities, not sure if it has something to do with that. Windows Tests are runnng fine though.

Run npm install -g @azure/static-web-apps-cli
(node:3792) ExperimentalWarning: CommonJS module /opt/hostedtoolcache/node/23.3.0/x64/lib/node_modules/npm/node_modules/debug/src/node.js is loading ES Module /opt/hostedtoolcache/node/23.3.0/x64/lib/node_modules/npm/node_modules/supports-color/index.js using require().
Support for loading ES Module in require() is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
npm warn deprecated [email protected]: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
npm warn deprecated [email protected]: Glob versions prior to v9 are no longer supported
npm warn deprecated [email protected]: Rimraf versions prior to v4 are no longer supported

changed 345 packages in 15s

72 packages are looking for funding
  run `npm fund` for details
(node:3822) ExperimentalWarning: CommonJS module /opt/hostedtoolcache/node/23.3.0/x64/lib/node_modules/npm/node_modules/debug/src/node.js is loading ES Module /opt/hostedtoolcache/node/23.3.0/x64/lib/node_modules/npm/node_modules/supports-color/index.js using require().
Support for loading ES Module in require() is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)

added 199 packages, and audited [20](https://github.com/CommunityToolkit/Aspire/actions/runs/12020126580/job/33508109977?pr=277#step:18:21)0 packages in 3s

42 packages are looking for funding
  run `npm fund` for details

2 vulnerabilities (1 moderate, 1 high)

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

@aaronpowell
Copy link
Member

It looks like we have a test that is hanging and causing the Linux runner to time out after several hours.

Assuming that these tests pass locally, this is going to be a fun one to try and resolve 🤣. Comparing the test results artifacts between Windows and Linux the only missing TRX file is the CommunityToolkit.Aspire.Hosting.EventStore.Tests, which suggests that the problematic tests are in that project.

The most likely candidates for the issue is going to be the tests in the EventStoreFunctionalTests.cs file. When I've seen this problem in the past it is related to the test waiting for something to complete that never completes but is still running "successfully" so a timeout on the async method doesn't work.

Here's the steps I'd go through to try and resolve it:

  1. Add a Skip attribute to all tests in that file and push
    1.1. If the runner still times out, also skip the test that use the example project - basically all tests that use Docker since they don't run on Windows
  2. Add one test back per push until you find the test that causes the runner to not exit

Once you know what test is the problematic one, we'll be able to help diagnose the underlying problem.

@fredimachado
Copy link
Contributor Author

All good, thanks for the suggestions. I'll try it.

@aaronpowell
Copy link
Member

All good, thanks for the suggestions. I'll try it.

Unfortunately, this isn't the first time I've seen runners hang 🤣

@aaronpowell
Copy link
Member

Well, looks like we found where the tests are that are the problem

@fredimachado
Copy link
Contributor Author

fredimachado commented Nov 28, 2024

It seems the we had to change permissions for the data bind mount to work on the Ubuntu build.
cc @aaronpowell @Alirexaa

image

@aaronpowell
Copy link
Member

It seems the we had to change permissions for the data bind mount to work on the Ubuntu build. cc @aaronpowell @Alirexaa

image

That's interesting, wonder why...

@aaronpowell
Copy link
Member

Just waiting for another review from @Alirexaa and it should be good to go

Copy link
Member

@Alirexaa Alirexaa left a comment

Choose a reason for hiding this comment

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

@fredimachado Please take a look at my comments.
Otherwise LGTM

@@ -120,16 +131,18 @@ public async Task WithDataShouldPersistStateBetweenUsages(bool useVolume)
}
else
{
//EventStore shutdown can be slightly delayed, so second instance might fail to start when using the same bind mount before shutdown.
await Task.Delay(TimeSpan.FromSeconds(5));
Copy link
Member

Choose a reason for hiding this comment

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

This could incur flakiness.

Related dotnet/aspire#4878

@@ -191,7 +204,7 @@ public async Task VerifyWaitForEventStoreBlocksDependentResources()
var resource = builder.AddEventStore("resource")
.WithHealthCheck("blocking_check");

var dependentResource = builder.AddEventStore("dependentresource")
var dependentResource = builder.AddContainer("nginx", "mcr.microsoft.com/cbl-mariner/base/nginx", "1.22")
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 a light (meaning small image size and fast startup) container image?
If it is we could use this image for dependent resources in health check tests like this test to speed up our tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, I used this specifically because it's lightweight and fast to start.
I've seen usages of this image for the same purpose in the main Aspire repo.

@Alirexaa
Copy link
Member

@aaronpowell, CodeQL is blocking me to merge PR.

@aaronpowell aaronpowell merged commit ea964f7 into CommunityToolkit:main Nov 29, 2024
8 checks passed
@aaronpowell
Copy link
Member

@aaronpowell, CodeQL is blocking me to merge PR.

Grr, I need to find out why that happens

@fredimachado
Copy link
Contributor Author

Thanks @aaronpowell and @Alirexaa for your support.

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.

EventStore Integration
3 participants