-
Notifications
You must be signed in to change notification settings - Fork 2
Eagerly load services #8
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
Eagerly load services #8
Conversation
| /// <remarks> | ||
| /// - <see cref="ContainerLifetime.Persistent"/>: Container survives application restarts (default for databases) | ||
| /// - <see cref="ContainerLifetime.Session"/>: Container is cleaned up when application stops (recommended for LocalStack) | ||
| /// - <see cref="ContainerLifetime.Transient"/>: Container is recreated on each run |
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.
Unrelated change, just spotted the doc comments refer to an option that doesn't exist
| } | ||
|
|
||
| private static IResourceBuilder<T> WithLocalStackHealthCheck<T>(this IResourceBuilder<T> builder, string[] services) where T : IResourceWithEndpoints | ||
| { |
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.
Help me here, I think I should delete most of this method and use what you already have with LocalStackConnectionStringAvailableCallback, but not sure the right way to reuse that here.
| @@ -1,14 +1,18 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
|
|
|||
| <Sdk Name="Aspire.AppHost.Sdk" Version="9.4.0" /> | |||
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.
The changes in this file is to allow inline creation of DistributedApplicationTestingBuilder, without a separate AppHost project.
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.
In this PR (#9), I added some new useful helpers to the Aspire.Hosting.LocalStack.Integration.Tests project and also reorganized the Lambda playground tests. Thought this might be helpful for you as well.
| using var cts = CancellationTokenSource.CreateLinkedTokenSource(parentCts.Token, TestContext.Current.CancellationToken); | ||
|
|
||
| #pragma warning disable CA1849 | ||
| await using var builder = DistributedApplicationTestingBuilder.Create("LocalStack:UseLocalStack=true"); |
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've decided that this is simple enough to not warrant a "playground" project and just create these builders inline
|
Hey @slang25, thanks for the PR! I'm a bit swamped this week, so I likely won't be able to take a look for another 2–3 days. I'll review it and get back to your questions as soon as I can. Really appreciate your contribution 🙏 |
|
Hey @slang25 , First off, thanks again for the effort 🙏 The eager-loading capability is genuinely useful. I went through your PR in detail and also poked around Aspire’s health-check docs a bit: https://learn.microsoft.com/en-us/dotnet/aspire/fundamentals/health-checks Based on that, I’ve got a few architectural suggestions that I think will make this even cleaner and easier to evolve:
I drafted something like this: private static IResourceBuilder<LocalStackResource> ConfigureHealthCheck(
IDistributedApplicationBuilder builder,
IResourceBuilder<LocalStackResource> resourceBuilder,
string[]? serviceNames)
{
var healthCheckName = $"{resourceBuilder.Resource.Name}_health";
var endpoint = resourceBuilder.Resource.GetEndpoint(LocalStackResource.PrimaryEndpointName);
builder.Services.AddHealthChecks().Add(new HealthCheckRegistration(
healthCheckName,
sp =>
{
// At this point, the endpoint should be allocated
if (!endpoint.IsAllocated)
{
// I'm not super sure about throwing an exception here
throw new InvalidOperationException($"LocalStack endpoint '{LocalStackResource.PrimaryEndpointName}' not yet allocated.");
}
var httpClientFactory = sp.GetRequiredService<IHttpClientFactory>();
var healthCheckUrl = $"{endpoint.Url}/_localstack/health";
// The health check decides internally whether to do lazy or eager checking
return new LocalStackHealthCheck(
httpClientFactory,
healthCheckUrl,
serviceNames);
},
failureStatus: null,
tags: new[] { "localstack", /* maybe "eager"/"lazy" */ }));
// Associate the health check with the resource
return resourceBuilder.WithHealthCheck(healthCheckName);
} |
|
|
||
| internal sealed class LocalStackHealthCheck(Uri uri, string[] services) : IHealthCheck, IDisposable | ||
| { | ||
| private readonly HttpClient _client = |
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 avoid instantiating HttpClient directly here. Instead, inject IHttpClientFactory via the constructor.
See my general comment: #8 (comment)
| #pragma warning disable CA2234 | ||
| using var response = await _client.GetAsync("_localstack/health", cancellationToken).ConfigureAwait(false); | ||
| #pragma warning restore CA2234 | ||
| if (response.IsSuccessStatusCode) |
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.
Consider a fail-fast approach here. We can invert the condition and return early to improve readability.
| using var response = await _client.GetAsync("_localstack/health", cancellationToken).ConfigureAwait(false); | ||
| #pragma warning restore CA2234 | ||
| if (response.IsSuccessStatusCode) | ||
| { |
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.
Somewhere after IsSuccessStatusCode, we could handle it like this:
// LAZY LOADING MODE: No specific services to check
if (services is null || services.Length == 0)
{
return HealthCheckResult.Healthy("LocalStack is healthy");
}That way, the lazy-loading case is explicitly covered.
Alternatively, instead of relying on the services array, it might be more robust to pass an explicit Lazy/Eager mode flag into the LocalStackHealthCheck class, so the behavior is intentional and less dependent on state.
| } | ||
|
|
||
| if (containerOptions.EagerLoadedServices.Count == 0) | ||
| { |
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 drop this block altogether, since both lazy and eager cases will be handled directly inside LocalStackHealthCheck
| { | ||
| resourceBuilder = resourceBuilder.WithEnvironment(key, value); | ||
| } | ||
|
|
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.
Somewhere around here we can register our HttpClient:
builder.Services.AddHttpClient("localstack");It might be cleaner to avoid using a magic string maybe move "localstack" into a constant. I’m not entirely sure which approach is better here, but leaning toward a constant for clarity.
| return HealthCheckResult.Unhealthy("LocalStack is unhealthy."); | ||
| } | ||
| #pragma warning disable CA1031 // Do not catch general exception types | ||
| catch (Exception ex) |
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.
Just nitpicking here. It would also be useful to handle cases like:
catch (HttpRequestException ex)
{
return HealthCheckResult.Unhealthy("LocalStack health check failed: network error", ex);
}
catch (TaskCanceledException ex) when (ex.InnerException is TimeoutException)
{
return HealthCheckResult.Unhealthy("LocalStack health check timed out", ex);
}This way we can capture network errors and timeouts explicitly, making the health check more easier to diagnose.
| return options; | ||
| } | ||
|
|
||
| private static IResourceBuilder<T> WithLocalStackHealthCheck<T>(this IResourceBuilder<T> builder, string[] services) where T : IResourceWithEndpoints |
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.
See my general comment: #8 (comment)
| @@ -1,14 +1,18 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
|
|
|||
| <Sdk Name="Aspire.AppHost.Sdk" Version="9.4.0" /> | |||
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.
In this PR (#9), I added some new useful helpers to the Aspire.Hosting.LocalStack.Integration.Tests project and also reorganized the Lambda playground tests. Thought this might be helpful for you as well.
|
I don’t have a definitive solution for this right now, but I do think we should document this feature somewhere. Not sure if the README is the best place for it. Open to suggestions, though! |
| } | ||
| else | ||
| { | ||
| resourceBuilder = resourceBuilder.WithEnvironment("EAGER_SERVICE_LOADING", "1"); |
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 think setting the env variables here is reasonable. That said, there’s a subtle edge case:
Inside AddLocalStack we already have:
// Add any additional environment variables
foreach (var (key, value) in containerOptions.AdditionalEnvironmentVariables)
{
resourceBuilder = resourceBuilder.WithEnvironment(key, value);
}If the same env variables are passed in both places, I’m not sure which one would win, probably the ones added last, but it’s not obvious.
I don’t have a perfect solution right now, but a couple of future options could be:
- Dropping
AdditionalEnvironmentVariablesentirely and consolidating everything into containerOptions. - Adding a validation step to detect overlapping variables and warn/fail early.
The first approach might be too restrictive, so I’m leaning toward the validation idea as a safer middle ground.
Either way, I don’t think we need to solve this in this PR, I’m not expecting that. I just wanted to share this thought for future consideration.
- Replace Gist-based badge system with BadgeSmith REST API - Implement HMAC-SHA256 authentication for secure badge updates - Add branch-specific badge support (master and feature branches) - Update CI/CD workflow to post test results via authenticated API - Refactor update-test-badge action to extract owner/repo/branch from context - Update README badges to use new BadgeSmith endpoints - Add Windows and macOS test badges to README - Remove Release Candidate status and warnings - Add comprehensive v9.5.0 CHANGELOG entry - Document SQS Event Source bug fix (#6, #9) - Document eager service loading feature (#7, #8) - Update LocalStack container to 4.9.1 - Update Aspire.Hosting to 9.5.0 - Credit contributors: @slang25 (eager loading), @Blind-Striker (bug fixes, infra)
|
Hey @slang25 👋 Just wanted to check in on this PR, I really appreciate the effort you’ve already put into it 🙌 I added some review comments a few days ago, but I completely understand if you have other priorities at the moment. I’m planning to cut a new release soon, and since this is a great addition, I’d love to include it. If you’re short on time, I can take it from here and finish up the remaining changes based on your implementation. You’ve already done the heavy lifting. Of course, I’ll keep your commits and full credit. No pressure at all, though. Just let me know what works best for you! 😊 |
…k improvements Complete implementation of eager loading feature with critical bug fixes: **Feature Implementation:** - Add EagerLoadedServices property to LocalStackContainerOptions - Automatically set EAGER_SERVICE_LOADING and SERVICES environment variables - Update health check to verify eagerly loaded services are running - Add environment variable collision detection for SERVICES/EAGER_SERVICE_LOADING **Critical Bug Fixes:** - Fix missing resourceBuilder assignment on line 200 (fluent API pattern) - Fix case-insensitive service name lookup in health check - Fix URL construction to handle trailing slashes **Health Check Improvements:** - Use IHttpClientFactory with proper timeout configuration - Add case-insensitive JsonObject key matching (StringComparison.OrdinalIgnoreCase) - Implement IDisposable pattern for proper resource cleanup - Add specific error handling for HttpRequestException and timeouts **Testing:** - Add comprehensive LocalStackHealthCheck unit tests (9 test cases) - Add integration tests for eager loading scenarios - Add tests for environment variable collision detection - Add tests for multiple services, empty lists, and edge cases **Documentation:** - Add Container Configuration section to README - Create comprehensive CONFIGURATION.md guide - Document lazy vs eager loading patterns - Add configuration patterns for Dev/CI/CD/Testing - Cross-reference official LocalStack documentation - Add troubleshooting guide Co-authored-by: slang25 <[email protected]> Closes #7 Refs #8
📝 Description
This PR adds the ability to eagerly load services in localstack.
Related Issue(s):
🔄 Type of Change
🎯 Aspire Compatibility
🧪 Testing
How has this been tested?
UseLocalStack())WithReference())Test Environment:
📚 Documentation
✅ Code Quality Checklist
🔍 Additional Notes
Breaking Changes:
If this is a breaking change, describe the impact and migration path for users.
Performance Impact:
Describe any performance implications of these changes.
Dependencies:
List any new dependencies or version changes.
🎯 Reviewer Focus Areas
Please pay special attention to:
📸 Screenshots/Examples
If applicable, add screenshots or code examples showing the changes in action.
By submitting this pull request, I confirm that: