8000 Add WaitBehavior to WaitForResourceHealthyAsync by mitchdenny · Pull Request #7650 · dotnet/aspire · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add WaitBehavior to WaitForResourceHealthyAsync #7650

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
Merged
merged 2 commits into from
Feb 18, 2025
Merged

Add WaitBehavior to WaitForResourceHealthyAsync #7650

merged 2 commits into from
Feb 18, 2025

Conversation

mitchdenny
Copy link
Member

Description

Added an overload to the WaitForResourceHealthyAsync method that allows us to specify the wait behavior if the resource fails to start. When set to StopOnDependencyFailure if the resource enters a failed to start state it will throw an exception.

Fixes #7601

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

@Copilot Copilot AI review requested due to automatic review settings February 17, 2025 22:33
@mitchdenny mitchdenny self-assigned this Feb 17, 2025
Copilot
Copilot AI reviewed Feb 17, 2025
View reviewed changes
Copy link
Contributor
@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

@mitchdenny mitchdenny added this to the 9.1 milestone Feb 17, 2025
@mitchdenny
Copy link
Member Author

@davidfowl @eerhardt this fixes #7601 however I am wondering if there is an API design issue here. We are adding an overload to WaitForResourceHealthyAsync to take a WaitBehavior enumeration but the value of StopOnDependencyFailure isn't exactly accurate since we aren't interrogating the dependency state in this case - we are just relying on the resources state (which may go into a failure state because its dependency failed).

I'm wondering whether we need either a new enumeration here or a new enumeration value. A new enumeration value would be a bit strange because then we would have enumeration values that only apply in certain contexts which is why I lead to a new enumeration - but WaitBehavior would be a good name for it.

Arguably we could rename StopOnDependencyFailure to just StopOnFailure (breaking change obviously).

@mitchdenny mitchdenny added area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication area-app-testing Issues pertaining to the APIs in Aspire.Hosting.Testing labels Feb 17, 2025
@mitchdenny mitchdenny changed the title Improve error handling in API requests Add WaitBehavior to WaitForResourceHealthyAsync Feb 18, 2025
Copilot
Copilot AI reviewed Feb 18, 2025
View reviewed changes
Copy link
Contributor
@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

@mitchdenny @Copilot
Update src/Aspire.Hosting/ApplicationModel/ResourceNotificationServic…
a0d8ff3 
…e.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@davidfowl
Copy link
Member

See main...davidfowl/wait-for-healthy

@mitchdenny
Copy link
Member Author

Merging this now we can come back with the enum rename since its a breaking change (this would just be one more place where it breaks but this is a less frequent usage scenario I think).

@mitchdenny mitchdenny merged commit e6aef9b into main Feb 18, 2025
70 checks passed
@mitchdenny mitchdenny deleted the mitchdenny/fix-7601 branch February 18, 2025 22:34
@mitchdenny
Copy link
Member Author

/backport to release/9.1

@github-actions GitHub Actions
Copy link
Contributor

Started backporting to release/9.1: https://github.com/dotnet/aspire/actions/runs/13401316958

@github-actions github-actions bot mentioned this pull request Feb 18, 2025
Merged
@davidfowl
Copy link
Member

@mitchdenny can you look at my branch and apply the delta?? I think this PR is incomplete

@mitchdenny
Copy link
Member Author

Ok

@mitchdenny mitchdenny mentioned this pull request Feb 19, 2025
Merged
18 tasks
@afscrome
Copy link
Contributor
afscrome commented Feb 19, 2025 •
edited
Loading

Merging this now we can come back with the enum rename since its a breaking change (this would just be one more place where it breaks but this is a less frequent usage scenario I think).

Isn't the WaitBehaviour enum new in 9.1, in which case we have a brief window to rename it without being a breaking change.

Edit: I see this got renamed in #7668

@afscrome afscrome mentioned this pull request Feb 20, 2025
Merged
18 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Mar 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

Copilot code review Copilot Copilot left review comments

@adamint adamint adamint approved these changes

@davidfowl davidfowl Awaiting requested review from davidfowl

@eerhardt eerhardt Awaiting requested review from eerhardt

Assignees

@mitchdenny mitchdenny

Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication area-app-testing Issues pertaining to the APIs in Aspire.Hosting.Testing
Projects
None yet
Milestone
9.1
Development

Successfully merging this pull request may close these issues.

WaitBehavior being ignored when waiting for resource to be healthy in a test.
4 participants
@mitchdenny @davidfowl @afscrome @adamint
0