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

Support deploying .dacpac from referenced NuGet packages #327

Open
jmezach opened this issue Dec 10, 2024 · 14 comments · May be fixed by #334
Open

Support deploying .dacpac from referenced NuGet packages #327

jmezach opened this issue Dec 10, 2024 · 14 comments · May be fixed by #334
Assignees
Labels
enhancement New feature or request integration A new .NET Aspire integration

Comments

@jmezach
Copy link
Contributor

jmezach commented Dec 10, 2024

Related to an existing integration?

Yes

Existing integration

SqlDatabaseProjects

Overview

A feature I would like to add to the existing SqlDatabaseProjects integration is the ability to publish a .dacpac from a NuGet package referenced by the AppHost project. At our company we already have an existing ecosystem of NuGet packages that contain a .dacpac and this is in fact one of the supported scenarios in MSBuild.Sdk.SqlProj. All these packages have essentially the same semantics, in the sense that the package contains a tools folder in which we can then find a .dacpac file that has the same name as the NuGet Package ID (ie. if the package is called MyDatabasePackage.nupkg there would be a tools\MyDatabasePackage.dacpac within that package). I'm not sure if Microsoft.Build.Sql has the same semantics though. That being said, we could probably support custom paths fairly easily by providing an overload that would allow one to specify the path within the package.

Usage example

I was thinking of something along the following lines:

var builder = DistributedApplication.CreateBuilder(args);

var server = builder.AddSqlServer("sql")
                    .AddDatabase("TargetDatabase");

builder.AddSqlPackage<Packages.SomeReferencedPackage>("some-referenced-package")
       .WithReference(server);

builder.Build().Run();

In case someone would want to customize the path to the .dacpac within the package we could provide an overload like so:

var builder = DistributedApplication.CreateBuilder(args);

var server = builder.AddSqlServer("sql")
                    .AddDatabase("TargetDatabase");

builder.AddSqlPackage<Packages.Microsoft_SqlServer_Dacpacs>("some-referenced-package", "tools/master.dacpac")
       .WithReference(server);

builder.Build().Run();

Breaking change?

No

Alternatives

We already support deploying a .dacpac directly from an arbitrary path using something like:

var builder = DistributedApplication.CreateBuilder(args);

var server = builder.AddSqlServer("sql")
                    .AddDatabase("TargetDatabase");

builder.AddSqlProject("some-referenced-package", "<full-path-to-package>.dacpac")
       .WithReference(server);

builder.Build().Run();

Although this works just fine, it will be hard for developers to figure out the right path to the .dacpac from a referenced NuGet package. These usually reside in the NuGet package cache, but will change every time the package reference is updated, make this code brittle.

Additional context

The proposed solution is akin to the existing solution for referenced projects, but this mechanism doesn't currently exist for referenced packages within Aspire afaik. I haven't looked in detail yet at how Aspire currently does this for projects, but maybe something similar could be created for referenced packages as well. Perhaps this could be achieved by adding some MSBuild logic to CommunityToolkit.Aspire.Hosting.SqlDatabaseProjects for the time being, which would generate a class similar to Projects that exposes IPackageMetadata? I would love some input from the Aspire folks on this.

/cc @ErikEJ

Help us help you

Yes, I'd like to be assigned to work on this item

@ErikEJ
Copy link
Contributor

ErikEJ commented Dec 10, 2024

Looks like Microsoft.Build.Sql uses same structure as the system .dacpacs, so assume it is also using the tools folder https://learn.microsoft.com/en-us/sql/tools/sql-database-projects/concepts/package-references?view=sql-server-ver16&pivots=sq1-visual-studio

@jmezach jmezach self-assigned this Dec 10, 2024
@jmezach
Copy link
Contributor Author

jmezach commented Dec 10, 2024

It looks like the Projects classes are being generated here. We could do something similar to generate Packages classes by looking at the PackageReferences that probably include some kind of opt-in metadata, like IsAspirePackageResource? Then I believe we could make this work end-to-end if we'd introduce IPackageMetadata that exposes both the PackageId and path to the package.

Or perhaps we would want to make this more specific for SQL database packages?

@jmezach
Copy link
Contributor Author

jmezach commented Dec 11, 2024

I did some early prototyping for this and already got quite far, see rr-wfm@a98c982. My main problem now is dealing with Central Package Management. When that is enabled the PackageReference doesn't have Version metadata, thus we cannot use that to get the correct path to the physical location. Not entirely sure how to fix that though, and it appears to be a problem for MSBuild.Sdk.SqlProj as well (since that uses pretty much the same logic to get the physical location of the package).

@jmezach
Copy link
Contributor Author

jmezach commented Dec 12, 2024

So it turns out that different generic constraints do not count towards having different overloads, so with the initially proposed design we would have two methods with the same signature which is not something C# allows. Therefore I've decided to rename the API to AddSqlPackage instead of AddSqlProject when dealing with packages. I've updated the design in the first post to reflect this.

It also turns out that the Microsoft.SqlServer.Dacpacs.Master package actually doesn't adhere to our semantics, in the sense that it contains master.dacpac instead of Microsoft.SqlServer.Dacpacs.Master.dacpac. So perhaps we should at least add the possibility of overriding the default?

As for the Central Package Management issue I've resolved that for now by adding the IsAspirePackageResource annotation to the PackageVersion element instead of the PackageReference. Not sure if that should be our end-state, but at least that works for now.

@aaronpowell Would love some feedback on this feature :)

@aaronpowell
Copy link
Member

I think it makes most sense to be using a separate method regardless, to clearly indicate the difference in the resolution (via package or via project).

For finding the package version when CPM is used, there must be a way to get the version number, as that's needed to be known at build time. Also, if you're using a version range in a PackageReference will that have an impact?

@jmezach
Copy link
Contributor Author

jmezach commented Dec 13, 2024

@aaronpowell There appears to be a _PackageDependenciesDesignTime item that contains almost all the information we need, including the local path on the machine and the version. However, that does not include the custom IsAspirePackageResource metadata anymore. Also it seems that the Identity of the elements in that Item are in the form of <PackageId>/<PackageVersion> so I don't know how I would go about resolving that based on the PackageReferences that have our metadata. We could of course generate IPackageMetadata for every package referenced by the AppHost, but I'm not sure if that's the right approach.

@aaronpowell
Copy link
Member

So it'd potentially add all the transient dependencies down the whole chain of the stack? that's probably not ideal.

I don't think it's inherently problematic putting it on the PackageVersion node, as long as we document the differences.

@jmezach
Copy link
Contributor Author

jmezach commented Dec 16, 2024

I agree, but even if we were to put it on the PackageVersion node we'd still have issues with resolving version ranges, so that only partially resolves our issue. Having done some more experimenting with this I think we need to somehow combine two Items (in MSBuild terminology) together to get the outcome we want. Essentially we'd have to go through all the _PackageDependenciesDesignTime items and then find the PackageReference item that has the same identity and which has the IsAspirePackageResource metadata set to true. However I'm not sure if that is something that can be done in purely MSBuild.

@jmezach jmezach linked a pull request Dec 16, 2024 that will close this issue
9 tasks
@jmezach
Copy link
Contributor Author

jmezach commented Dec 16, 2024

Okay, I did some more experimenting and I think I've figured it out now. See #334 for a first draft of this new feature. I guess I just got hit by dotnet/msbuild#2835 while developing this ;).

Still need to write some tests though.

@jmezach
Copy link
Contributor Author

jmezach commented Dec 17, 2024

@ErikEJ @aaronpowell Right, I've added some unit tests to the PR. Adding an integration test is going to be a bit harder, because that would require an actual NuGet package with a .dacpac that we can deploy. I know there is the Microsoft.SqlServer.Dacpacs.Master package, but that contains the master database that we can't really deploy to a SQL Server. I don't know of any other NuGet packages that have .dacpac's in them to be honest.

What we've done for MSBuild.Sdk.SqlProj is that we basically build a NuGet package as part of the CI pipeline and output it to a pre-defined folder on disk and then add that folder as a NuGet package source and install the package from there. That does add quite a bit of complexity to the pipeline however. Not sure if that's a route we'd want to take here?

@ErikEJ
Copy link
Contributor

ErikEJ commented Dec 17, 2024

@jmezach I can publish Chinook as a NuGet package, if that would help?

@ErikEJ
Copy link
Contributor

ErikEJ commented Dec 17, 2024

@jmezach Created a package using our latest template - interesting dependency tree?

Maybe we should improve this in the SDK or the template?

Image

@ErikEJ
Copy link
Contributor

ErikEJ commented Dec 17, 2024

@jmezach
Copy link
Contributor Author

jmezach commented Dec 17, 2024

@jmezach Created a package using our latest template - interesting dependency tree?

Maybe we should improve this in the SDK or the template?

Image

That sounds like a bug in MSBuild.Sdk.SqlProj that we should probably fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request integration A new .NET Aspire integration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants