-
Notifications
You must be signed in to change notification settings - Fork 597
Throw exception in WaitForResourceAsync
and WaitForResourceHealthyAsync
when resource does not exist
#8468
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
base: main
Are you sure you want to change the base?
Throw exception in WaitForResourceAsync
and WaitForResourceHealthyAsync
when resource does not exist
#8468
Conversation
WaitForResourceAsync
and WaitForResourceHealthyAsync
when resource does not exist
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.
Pull Request Overview
This PR adds resource existence validation for asynchronous wait methods in ResourceNotificationService and includes unit tests to verify that an InvalidOperationException is thrown when a non-existent resource is queried.
- Added resource existence checks before waiting in ResourceNotificationService.
- Added unit tests in ResourceNotificationTests to cover scenarios where the resource does not exist.
- Included additional using directives for dependency injection and health checks in the tests.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
tests/Aspire.Hosting.Tests/ResourceNotificationTests.cs | Added unit tests to verify that invalid resource names trigger exceptions. |
src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs | Added validation that throws an exception if a resource does not exist. |
|
||
if (!resourceExist) | ||
{ | ||
throw new InvalidOperationException($"Resource with name '{resourceName}' not found."); |
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.
Should this only be done if DefaultWaitBehaviour == WaitBehavior.StopOnResourceUnavailable
, as it is potentially possible that the resource could show up later. (Perhaps if WaitBehavior is something else, it is worth logging something to highlight that the resoruce doesn't exist, even if it doesn't blow up completely)
On the surface this sounds reasonable. Although the thing that gives me pause is around writing test cases where you might have resources added to the model dynamically after startup. When writing a test case you might opt the wait for the resource before it even exists, but then put a timeout in place. |
Description
This pull request includes changes to the
ResourceNotificationService
class to add validation for resource existence and corresponding unit tests to ensure this validation works correctly. The most important changes are listed below:Validation for resource existence:
src/Aspire.Hosting/ApplicationModel/ResourceNotificationService.cs
: Added checks to verify if a resource exists before proceeding with theWaitForResourceAsync
andWaitForResourceCoreAsync
methods. If the resource does not exist, anInvalidOperationException
is thrown. [1] [2]Unit tests for resource existence validation:
tests/Aspire.Hosting.Tests/ResourceNotificationTests.cs
: Added new unit tests to ensure thatWaitForResourceAsync
andWaitForResourceHealthyAsync
methods throw anInvalidOperationException
when the resource name does not exist.Additional imports for testing:
tests/Aspire.Hosting.Tests/ResourceNotificationTests.cs
: Added necessary imports for dependency injection and health checks in the test file.Fixes #8147
Checklist
<remarks />
and<code />
elements on your triple slash comments?breaking-change
template):doc-idea
template):