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

Generate metadata in manifest. #1619

Merged
merged 5 commits into from
Feb 4, 2024
Merged

Conversation

mitchdenny
Copy link
Member

@mitchdenny mitchdenny commented Jan 10, 2024

Fixes: #1959

This PR adds infrastructure to support emitting a "metadata": property for resources in the manifest. The usage is as follows:

var builder = DistributedApplication.CreateBuilder(args);
builder.AddProject<Projects.MyApp>("myapp")
       .WithMetadata("azd-identity", "guid-for-pre-existing-identity");

The manifest would look like this:

{
  "resources": {
    "myapp": {
      // some properties omitted for brevity.
      "type": "project.v0",
      "metadata": {
        "azd-identity": "guid-for-pre-existing-identity"
     }, 
   }
  }
}

Metadata properties use the JSON serializer on the value field so they can be simple types such as strings, booleans, and integers, but they can also be complex objects.

Microsoft Reviewers: Open in CodeFlow

@mitchdenny mitchdenny self-assigned this Jan 10, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jan 10, 2024
/// <summary>
/// Value of metadata entry.
/// </summary>
public object Value { get; } = value;
Copy link
Member

Choose a reason for hiding this comment

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

string?

Copy link
Member Author

Choose a reason for hiding this comment

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

On hold for now but based on latest offline chat we might still allow object serialization.

@davidfowl davidfowl added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 10, 2024
@mitchdenny mitchdenny mentioned this pull request Jan 29, 2024
@mitchdenny
Copy link
Member Author

mitchdenny commented Feb 3, 2024

@davidfowl merge conflict resolved. This metadata PR was ready to go in unless you can spot some issues with the current implementation.

@mitchdenny mitchdenny removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 3, 2024
@mitchdenny mitchdenny added this to the preview 4 (Mar) milestone Feb 3, 2024
foreach (var metadataAnnotation in metadataAnnotations)
{
Writer.WritePropertyName(metadataAnnotation.Name);
JsonSerializer.Serialize(Writer, metadataAnnotation.Value);
Copy link
Member

Choose a reason for hiding this comment

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

RIP AOT 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

We can probably inject a serializer via DI here but TBH it don't think anyone would be doing AOT on their app host, given the product we are shipping for GA there is no reason to wait for the native build when you just want to launch the app to either debug locally or produce a manifest.

/// <returns>Resource builder.</returns>
public static IResourceBuilder<T> WithMetadata<T>(this IResourceBuilder<T> builder, string name, object value) where T: IResource
{
var existingAnnotation = builder.Resource.Annotations.OfType<ManifestMetadataAnnotation>().SingleOrDefault(a => a.Name == name);
Copy link
Member

Choose a reason for hiding this comment

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

This is going to end up killed our perf when we do testing 😄.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can think of a many over engineered solutions which I'll be happy to implement once we get some good data on where our perf pain is.

@mitchdenny mitchdenny merged commit d57a54c into main Feb 4, 2024
8 checks passed
@mitchdenny mitchdenny deleted the mitchdenny/manifest-metadata branch February 4, 2024 20:30
radical pushed a commit to radical/aspire that referenced this pull request Feb 6, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Apr 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WithMetadata support
2 participants