-
Notifications
You must be signed in to change notification settings - Fork 147
Make SqlProject behave more like built in resources #994
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
Conversation
- Rework Sql Project resource to let Aspire handle the waiting, rather than handling it via a custom implementation - Refactor Publishing to not `Exited` with an exit code rather than `FailedToStart` if the publishing process started, but then failed. - Use Standard Resource states - Pending / Not Started --> Let aspire handle it - Publishing --> Running - Exceptions during the publish process now become `Exited` - Add icon for database projects - Add resource properties for SqlPackage projects - in particular the state which will show up on the dashboard. In particular this involves removing any custom publishing of the `ResourceReadyEvent` as this explicitly called out as something that should not be done: > Important: Developers should not manually publish the ResourceReadyEvent. Aspire manages the transition to the ready state based on the presence and outcome of health checks. Manually firing this event can interfere with the orchestration logic. https://github.com/dotnet/aspire/blob/main/docs/specs/appmodel.md#resource-health The key to getting aspire to do the waiting for us is to use `OnInitialize` to kick off our process. See the talking clock example at https://github.com/dotnet/aspire/blob/main/docs/specs/appmodel.md#example-custom-resource---talking-clock for more details. One thing I'm not entirely sure of is whether or not a `SqlProject` resource was intended to deploy multiple bacpacks from a signle resource. The current implemention look. If this is not intended, we can get rid of the `DacpackTargetAnnotation` and inline those properties on the resource itself. If this was intended, we can make `DacpackTargetAnnotation` public.
|
Looks like a test time out... What do you mean by your question re deploy multiple? Do you mean deploy multiple .dacpacs against the same database? I think that would be a valid scenario. |
|
Sorry, was in a bit of a rush before going out for the day, let me look at the tests For the multiple scenario, what should happen with the following: .WithDacpac("foo.bacpac")
.WithDacpac("baz.bacpac")Should this mean one |
|
Fixed the tests (although unrelated SurrealDb tests are failing). Also realise I misunderstood the resource lifecycle on my first pass, so my previous question was irrelevant. I've updated the Pr description to describe the Pr with the latest changes. |
aaronpowell
left a comment
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.
tests/CommunityToolkit.Aspire.Hosting.SqlDatabaseProjects.Tests/AppHostTests.cs
Outdated
Show resolved
Hide resolved
|
@afscrome Great work, I will pull the PR and do some smoke testing - but LGTM |
|
@afscrome @aaronpowell @jmezach I have smoke tested, works just as expected, and with much better state indications during launch than before. |
|
I also ran a couple of tests and seems to work as advertised now. Thanks @afscrome for this! |
Closes #942 (Fixing that issue wasn't a specific goal of this Pr, but it just so happens to fix it).
Update Dac Pack resources to better follow aspire resource conventions
Exitedwith an exit code, rather thanFailedtoStartIn particular this involves removing any custom publishing of the
ResourceReadyEventas this explicitly called out as something that should not be done:The key to getting aspire to do the waiting for us is to publish the
BeforeStartevent to kick off our process, and wait for that to process. See the talking clock example at https://github.com/dotnet/aspire/blob/main/docs/specs/appmodel.md#example-custom-resource---talking-clock for more details.This did involve making sure to not wait on the target database to avoid a deadlock. Previously
WithReferenceeffectively didBut this is a circular dependency.
WaitFordoesn't unblock until all ResourceReady events have completed. But theOnResourceReadyhandler needed to block until all the dacpac's dependencies were healthy. For dacpac's purposes, theWaitFor(target)was unnecessary.PR Checklist
The change of resource states is be a breaking change if any consumers are doing any custom waiting on those states. Some (but not all) of these state changes are required to let aspire handle the Waiting for us, but I felt it was easier to bring everything inline, rather than just some.
Other information
I'm not entirely sure if
OnResourceReadyis the right place to execute the dacpacs, as it blocks the database from going healthy until t's complete. Whilst I'm sure this is often the behaviour you want, I'm not sure it's always what is wanted. (WithExplicitStartis a clear example of this, but I can imagine some other users may not want to be blocking). But this is where execution was being done prior to now, so things are no worse with this Pr than they were before.