Skip to content

Conversation

mitchdenny
Copy link
Member

@mitchdenny mitchdenny commented Nov 28, 2023

Fixes #769

This PR adds an extension method to executable resource builders which flags that when emitted to the manifest they should be emitted as a dockerfile.v0 resource.

  • Basic emission logic for dockerfile.v0
  • Unit tests for manifest publishing
  • Remove changes made to sample (just used for illustration purposes, won't be merged)

Given the following code:

var builder = DistributedApplication.CreateBuilder(args);

builder.AddAzureProvisioning();

builder.AddNpmApp("nodeapp", "..\\nodeapp")
    .WithServiceBinding(5555, scheme: "http", env: "PORT")
    .WithDockerfile();

The following manifest snippet will be produced:

{
  "resources": {
    "nodeapp": {
      "type": "dockerfile.v0",
      "path": "..\\nodeapp\\Dockerfile",
      "context": "..\\nodeapp",
      "env": {
        "NODE_ENV": "development",
        "PORT": "{bindings.http.port}"
      },
      "bindings": {
        "http": {
          "scheme": "http",
          "protocol": "tcp",
          "transport": "http",
          "containerPort": 5555
        }
      }
    }
}

@mitchdenny mitchdenny requested a review from davidfowl November 28, 2023 04:48
@ghost ghost added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Nov 28, 2023
@mitchdenny
Copy link
Member Author

@ellismg and @rajeshkamal5050 for thoughts on this from the AZD perspective.

@davidfowl
Copy link
Member

Since we're making bigger API changes I think we should make more to set out selves up for a better future:

  1. Expose an extension method that adds the ManifestCallbackAnnotation (if we had that we could avoid the break here)
  2. The ManifestPublisherContext should have the manifest output path as context
  3. We should consider flowing the resource itself to the ManifestPublisherContext (closures feel sorta bad).

@prom3theu5
Copy link
Contributor

Added support for the dockerfile component to Aspir8 based on the above manifest entry. Will run through some tests over next few days - and follow this PR to see if there are any changes.

Atm - i've implemented it using the ContainerRegistry setup during the init command, along with resource name and latest tag on each build.
Tag will be made customizable though, and container registry overridable as a cli option - just not today, i'm a busy bee today 😛

@mitchdenny
Copy link
Member Author

i'm a busy bee today 😛

Appreciate everything you are doing truly. You're making an awesome contribution to the project.

@mitchdenny
Copy link
Member Author

2. The ManifestPublisherContext should have the manifest output path as context

It is ... check the ManifestPath property.

3. We should consider flowing the resource itself to the ManifestPublisherContext (closures feel sorta bad).

Not sure how I feel about this. If we go down this route I'll need to rework the callback methods (fine) but the very first thing I'll need to do on each of those callback methods is cast a non-generic IResource to the specific resource type that we are working within. I feel like the current capture approach is actually more ergonomic.

@DamianEdwards
Copy link
Member

I'm not sure it's super clear that the .WithDockerFile() call only applies to the manifest. I look at that and think it's going to build and run a container when launched by the AppHost during local dev. Have we considered making it more explicit?

@mitchdenny
Copy link
Member Author

I'm not sure it's super clear that the .WithDockerFile() call only applies to the manifest. I look at that and think it's going to build and run a container when launched by the AppHost during local dev. Have we considered making it more explicit?

Yeah I struggle with this too. The With verb doesn't quite work. I'm considering we do something like: AsDockerfileInManifest(...) but it seems a little too wordy? Thoughts?

@DamianEdwards
Copy link
Member

Yeah I struggle with this too. The With verb doesn't quite work. I'm considering we do something like: AsDockerfileInManifest(...) but it seems a little too wordy? Thoughts?

You know what, I think I prefer that, or even WithDockerfileInManifest()

@mitchdenny
Copy link
Member Author

mitchdenny commented Nov 28, 2023

I have a weakly held opinion that As is better than With in this case, but I don't want to introduce a new prefix unnecessarily.

@DamianEdwards
Copy link
Member

I have a weakly held opinion that As is better than With in this case, but I don't want to introduce a new prefix unnecessarily.

Yep I totally see your point and I could see us adopting the As prefix for "transform" operations like this. Do what you feel is best for now 😄

@mitchdenny mitchdenny merged commit ff5562c into main Nov 29, 2023
@mitchdenny mitchdenny deleted the add-dockerfile branch November 29, 2023 04:06
@mitchdenny mitchdenny added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Nov 29, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Apr 28, 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 breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add docker file manifest resource type
4 participants