Skip to content

Made some dapr improvements based on feedback#1133

Merged
karolz-ms merged 1 commit intomainfrom
davidfowl/dapr-improvements
Nov 30, 2023
Merged

Made some dapr improvements based on feedback#1133
karolz-ms merged 1 commit intomainfrom
davidfowl/dapr-improvements

Conversation

@davidfowl
Copy link
Copy Markdown
Contributor

@davidfowl davidfowl commented Nov 30, 2023

  • Default appid to the resource name
  • Name the dapr side car resource name-dapr
  • Look for the side car annotation on all resources.

Fixes #1091
Fixes #926

- Default appid to the resource name
- Name the dapr side car resource name-dapr
- Look for the side car annotation on all resources.
@ghost ghost added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Nov 30, 2023
@davidfowl davidfowl requested a review from mitchdenny November 30, 2023 05:14
}

var resource = new ExecutableResource(appId, fileName, workingDirectory, daprCommandLine.Arguments.ToArray());
var daprSideCar = new ExecutableResource($"{resource.Name}-dapr", fileName, appHostDirectory, daprCommandLine.Arguments.ToArray());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if it'd make sense to add an option to allow setting the resource name in case {resource}-dapr isn't desired.

}

var resource = new ExecutableResource(appId, fileName, workingDirectory, daprCommandLine.Arguments.ToArray());
var daprSideCar = new ExecutableResource($"{resource.Name}-dapr", fileName, appHostDirectory, daprCommandLine.Arguments.ToArray());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Originally the sidecar was run in the project directory to mimic how the sidecars would be started when using Dapr run files (in order for the sidecar's (optional) Command property, which was often dotnet run to make sense). The user can still specify a command in the sidecar options, though it's probably of little use when using Aspire's orchestration.

It's probably ok to generally use the app host directory, since we've made all the important paths relative to it anyway, but it will be a point of inconsistency between Dapr (by itself) and Dapr (in Aspire). For non-project resources (like executables), the app host directory is probably the only real option. I suppose we could add a WorkingDirectory sidecar option if/when customers have a need.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we can make it work for projects and executables, but containers don’t have natural working directory. What does looking based on this directory ?

Copy link
Copy Markdown
Member

@philliphoff philliphoff left a comment

Choose a reason for hiding this comment

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

Agree that it makes sense to use the Aspire name for the "parent" resource as the default Dapr application ID.

@karolz-ms karolz-ms merged commit a658d70 into main Nov 30, 2023
@karolz-ms karolz-ms deleted the davidfowl/dapr-improvements branch November 30, 2023 22:46
andrevlins pushed a commit to andrevlins/aspire that referenced this pull request Dec 3, 2023
- Default appid to the resource name
- Name the dapr side car resource name-dapr
- Look for the side car annotation on all resources.
@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.

Executable with WithDaprSidecar does not start sidecar Dapr Sidecar resource name should not be appId

3 participants