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

Added support for node and npm based projects #1033

Merged
merged 11 commits into from Nov 28, 2023
Merged

Conversation

davidfowl
Copy link
Member

@davidfowl davidfowl commented Nov 25, 2023

  • Added AddNodeApp and AddNpmApp extension methods
  • Flow the project's working directory into the host in 2 ways, via an attribute and as another project in the list. By default, executable's working directory are relative to the AppHost's project directory (this is what the attribute is used for).
  • Added support for flowing the dynamically chosen port to the application by specifying a "portEnvVar" setting on the ServiceBindingAnnotation.
  • Added IResourceWithServiceDiscovery which allows custom resources to export service discovery information.
  • This test relies on node being installed.
  • Added node.v0 resource for node apps
  • Improve support for executable resources in the manifest.

TODO:

  1. Test for npm based project.
  2. Skip tests if node isn't on the path.
  3. Figure out what to do for deployment? Should we model this as a node.v0 resource? That would match what we do for .NET projects...

Fixes #1017

PS: Fixing some of these issues (flowing port to the app and fixing working directories makes it easier to execute other project types in general). AddNpmApp and AddNodeApp are very thin wrappers now.

- Added AddNodeApp and AddNpmApp extension methods
- Flow the project's working directory into the host in 2 ways, via an attribute and as another project in the list. By default, executable's working directory are relative to the AppHost's project directory (this is what the attribute is used for).
- Added support for flowing the dynamically chosen port to the application by specifying a "portEnvVar" setting on the ServiceBindingAnnotation.
- This test relies on node being installed.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Nov 25, 2023
- This allows custom resources to opt-in to service discovery exports.
- Removed extra generic argument from WithReference
@mitchdenny
Copy link
Member

Overall this looks pretty solid.

@mitchdenny
Copy link
Member

3. Figure out what to do for deployment? Should we model this as a node.v0 resource? That would match what we do for .NET projects...

I think if we go down the path of emitting a node.v0 resource that we'll have to give some kind of guidance on what we expect tools to do with it. For example, if azd or aspirate encounter a node.v0 resource should then look for a Dockerfile in the same path as package.json? Would there be an explciit override in package.json to specify the location of the Dockerfile?

@davidfowl
Copy link
Member Author

I think if we go down the path of emitting a node.v0 resource that we'll have to give some kind of guidance on what we expect tools to do with it. For example, if azd or aspirate encounter a node.v0 resource should then look for a Dockerfile in the same path as package.json? Would there be an explciit override in package.json to specify the location of the Dockerfile?

Right, I think this is what we need to decide. Today for project.v0 resources (maybe this should be a dotnet.v0 resource) we recommend people use container builds, but that's all dependent on the target environment. You could imagine it meaning do a dotnet publish and zip the results and upload it to the target (It could mean use the nodejs build pack... etc)

I think docker file support needs to be an explicit call.

@mitchdenny
Copy link
Member

Regarding renaming project.v0 to dotnet.v0 ... I think this might make sense. Although we'd have to consider how things like *.esproj files might be handled if we want to explicitly support them. Maybe AddProject interrogates the project metadata and calls AddDotnet or AddNpm as appropriate.

What I mean about Docker support though is that I think as a framework orientated towards cloud native development we assume that if you have a reosurce like Node or Python apps that there will be a Docker build involved in deployment.

@davidfowl
Copy link
Member Author

What I mean about Docker support though is that I think as a framework orientated towards cloud native development we assume that if you have a reosurce like Node or Python apps that there will be a Docker build involved in deployment.

Right, but I don't think we should assume that. Maybe the deployment tool can make that assumption. If you see a node.v0 resource, or a python.0 resource, we point you at the source location and the tool can determine that it requires a docker file to produce asserts for deployment (or not).

- Renamed PortEnvVar to env
- Added manifest resource for node apps (node.v0)
- Cleaned up executable support
Copy link
Member

@mitchdenny mitchdenny left a comment

Choose a reason for hiding this comment

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

Assuming WriteNodeApp is temporarily in ManifestPublisher until the ManifestPublisherCallbackAnnotation is updated to take a context.

@davidfowl davidfowl merged commit f8c3c9b into main Nov 28, 2023
3 checks passed
@davidfowl davidfowl deleted the davidfowl/node-support branch November 28, 2023 00:02
@@ -63,13 +63,51 @@ public class ]]>%(ClassName)<![CDATA[ : IServiceMetadata
</ItemGroup>
</Target>

<Target Name="CreateProjectMetadata">
Copy link
Member Author

Choose a reason for hiding this comment

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

@baronfel can you give me an after the fact review on this?

Copy link
Member

Choose a reason for hiding this comment

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

This all seems ok to me, seems straightforward enough based on the existing patterns.

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add AddNpmApp and AddNodeApp to Aspire.Hosting
3 participants