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

[Feature] Dapr - Azure extensions + Redis configuration #349

Open
FullStackChef opened this issue Dec 24, 2024 · 29 comments · May be fixed by #371
Open

[Feature] Dapr - Azure extensions + Redis configuration #349

FullStackChef opened this issue Dec 24, 2024 · 29 comments · May be fixed by #371
Labels
awaiting response Waiting for the author of the issue to provide more information or answer a question help wanted Extra attention is needed integration A new .NET Aspire integration

Comments

@FullStackChef
Copy link

.NET Aspire issue link

No response

Overview

  1. Dapr–Azure Integration
  • Simplifies the configuration of Azure resources for Dapr.
  • Uses Aspire’s resource builder pattern to configure the necessary infrastructure when you publish your application.
  • Eliminates the need to maintain additional Dapr component manifests.
  1. Azure Redis Extension : Builds on top of the Dapr–Azure integration to configure Redis as a Dapr state store.

Usage example

var builder = DistributedApplication.CreateBuilder(args);


var redisState = builder.AddAzureRedis("redisState").RunAsContainer();

// This currently only effects publishing
// local development still uses dapr redis state container
var daprState = builder.AddDaprStateStore("daprState")
    .WithReference(redisState); // <= This instructs the application to configure dapr to use the redis state store

// API does not provide any functional example of Dapr - it simply demonstrates referencing the dapr state
var api = builder.AddProject<Projects.CommunityToolkit_Aspire_Hosting_Dapr_AzureRedis_ApiService>("example-api")
    .WithReference(daprState)
    .WithDaprSidecar();

builder.Build().Run();

Additional context

I have this on a branch ready to go into a PR

Help us help you

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

@aaronpowell aaronpowell added integration A new .NET Aspire integration help wanted Extra attention is needed awaiting response Waiting for the author of the issue to provide more information or answer a question labels Dec 29, 2024
@aaronpowell
Copy link
Member

I'm unfamiliar with how Dapr works, but if I'm understanding, this is a proposal to add this API:

var daprState = builder.AddDaprStateStore("daprState")
    .WithReference(redisState);

And that gives you a resource you can provide to another resource, such as the API project in the above sample, to have an opinionated pre-configured Dapr integration. Is that right?

@davidfowl
Copy link

Feels like these should go with the dapr integration

@FullStackChef
Copy link
Author

I guess there are two parts to the proposal the first part (CommunityToolkit.Aspire.Dapr.AzureExtensions) adds a set of functions that can be used to configure the dapr resources directly from the apphost rather than requiring supplementary yaml files for the dapr config

The second part is a specific implementation as above for redis (CommunityToolkit.Aspire.Dapr.AzureRedis) which adds the

.WithReference(redisState);

i.e
var daprState = builder.AddDaprStateStore("daprState")

already exists as part of Aspire.Hosting.Dapr but requires additional configuration based on yaml files. This uses withreference to configure the dapr state based on the

var redisState = builder.AddAzureRedis("redisState")

above.

Note the current implementation has no bearing on local development, only on publishing

Two readme files might help

https://github.com/FullStackChef/CommunityToolkit-Aspire/blob/dapr-azure-hosting-ext/src/CommunityToolkit.Aspire.Hosting.Dapr.AzureExtensions/README.md

https://github.com/FullStackChef/CommunityToolkit-Aspire/blob/dapr-azure-hosting-ext/src/CommunityToolkit.Aspire.Hosting.Dapr.AzureRedis/README.md

@FullStackChef
Copy link
Author

Feels like these should go with the dapr integration

Couple of things I considered here:

  • Pretty certain that would simplify parts of the integration.
  • The current Dapr integration as I understand it is not Azure specific - (I think my integration is - or at least I haven't considered non azure since I live in azure)
  • Perhaps it's not one or the other? Maybe AzureExtensions should sit with Dapr integration while specific implemenations are toolkit? The reason I created this as two libraries is that otherwise you have to reference every possible azure hosting package in the toolkit where as this approach allows you just to install redis for redis if that makes sense

@aaronpowell
Copy link
Member

Cool, that all makes sense.

The question on whether this should be in the Aspire repo (likely as new packages due to the Azure dependency) or the Community Toolkit is something that we'd need the Aspire team to weigh in on.

@FullStackChef
Copy link
Author

@davidfowl / @aaronpowell - Is there anything I can / need to do to progress this? I realise the majority of people are probably still on xmas break - I was hoping to get this at least mostly done before I head back to work next week

@aaronpowell
Copy link
Member

I think, unfortunately, the people that I need input on regarding whether this should be in the Community Toolkit or the Aspire Dapr package are on leave still (@davidfowl, @mitchdenny, etc.).

@davidfowl
Copy link

davidfowl commented Jan 2, 2025

I think we have 3 options:

  1. Keep the dapr integration as part of the core and evolve it there.
  2. Move the entire dapr integration to the community toolkit and evolve it here.
  3. Move the dapr integration to dapr .NET client team.

I've spoken to @philliphoff and the dapr team about 3, but it feels like 2 feels like reasonable approach. The code above is exactly the vision for dapr component aspire integration. On top of that it should work in a first-class way with dapr on ACA (for the supported components there). We should have enough primitives exposed to make this possible to support without changes to the core itself.

The last complication is that we have Aspire.Hosting.Dapr today. Would we move that as is or do we deprecate this package and move to CommunityToolkit.Aspire.Hosting.Dapr. I think this package would be better off in the hands of strong community leadership that is using it and can help it evolve quickly. There's LOTS of potential to simplify the workflow here and it's just not a priority for the core team.

cc @eerhardt @cecilphillip @paule96 @atrauzzi @oising

@oising
Copy link

oising commented Jan 2, 2025

My vote is on #2

@davidfowl
Copy link

davidfowl commented Jan 2, 2025

I’m leaning that way too, we need some maintainers to put their hands up

Image

@FullStackChef
Copy link
Author

If migration is the path forward - I'm happy to do the work provided as part of my what I've already done. At this point I feel like I've spent enough time staring at the code to understand it. 😬

@eerhardt
Copy link

eerhardt commented Jan 2, 2025

I would vote for (2) as well, if (3) isn't possible.

we need some maintainers to put their hands up

I think this is the key. As you mention above "and it's just not a priority for the core team." makes it hard to evolve in the dotnet/aspire repo, as the core team would need to review/approve PRs and triage issues.

The last complication is that we have Aspire.Hosting.Dapr today. Would we move that as is or do we deprecate this package and move to CommunityToolkit.Aspire.Hosting.Dapr.

Is the rename 100% necessary? We could move the package as-is to this repo, if we wanted.

@paule96
Copy link

paule96 commented Jan 2, 2025

Yeah I can just agree with @davidfowl. Was thinking the same when I opened in the main repo the idea/draft PR of a better integration of dapr. (dotnet/aspire#6965)

@FullStackChef are you in the dapr discord? Maybe we can connect. Because I see we both have similar ideas and potentially implement the same things.

@davidfowl
Copy link

Is the rename 100% necessary? We could move the package as-is to this repo, if we wanted.

@aaronpowell How do you feel about that? Does it muddy things? I think it does a bit but I wouldn't die on this hill

OK executive decision, we move the dapr integration to the community toolkit.

@philliphoff wrote the integration and is one of the dapr sdk maintainers so I would like to him to chime in at the very least on the vision 😄

@FullStackChef
Copy link
Author

Yeah I can just agree with @davidfowl. Was thinking the same when I opened in the main repo the idea/draft PR of a better integration of dapr. (dotnet/aspire#6965)

@FullStackChef are you in the dapr discord? Maybe we can connect. Because I see we both have similar ideas and potentially implement the same things.

@paule96 I am in the dapr discord. collaboration sounds good to me.

@davidfowl @eerhardt
What is the value of not changing the name? it seems unlikely that this would be just a lift & shift - looking at the csproj for the library there is at least one thing that is going to need to be revisited

<Compile Include="$(SharedDir)Utf8JsonWriterExtensions.cs" Link="Utils\Utf8JsonWriterExtensions.cs" />

@davidfowl
Copy link

Reasons off the top of my head:

  • Versioning, the ship cycle will diverse from the rest of the toolkit.
  • Docs location, this should be part of the community toolkit docs
  • Optics - It should be seen as part of the toolkit

Like I said, I won't die on a hill for this one though.

@eerhardt
Copy link

eerhardt commented Jan 2, 2025

rename

Yeah, I'm not convinced 100% either way is the "right way". Just having a discussion and trying to get my head around the thinking.

If we were to have started this project in the CommunityToolkit repo, we would have used the CommunityToolkit prefix to the package - like all the other packages.

But, if we were to follow option (3) above and move the dapr Aspire integration to dapr .NET client team, we definitely wouldn't use the CommunityToolkit prefix.

This is one of the reasons I dislike having ownership in library/package names. Ownership of a library can change over time, and having to rename the package just because the ownership changed isn't ideal.

When we moved the AWS Aspire integration to a new repo, we didn't need to rename the package. aws/integrations-on-dotnet-aspire-for-aws#1.

But maybe the thing that makes this situation special is that all the packages coming out of the CommunityToolkit are explicitly named CommunityToolkit. It's a special case?

@FullStackChef
Copy link
Author

@davidfowl I was asking the opposite - sorry poor semantics, My question is what is the value of leaving the name as is? It seems like a pretty trivial amount of effort to change?

@eerhardt I don't disagree on the naming convention and ownership - except to say that I feel like consistency is better than perfection - the rest of the community toolkit is named as such it seems strange to name something differently

@eerhardt
Copy link

eerhardt commented Jan 2, 2025

My question is what is the value of leaving the name as is?

  • The existing users get prompted to update to the new version when it is released. They don't need to somehow find out that the package has been "moved" from under them.
  • We don't have to deprecate an existing package.

@davidfowl
Copy link

I think that’s a feature, letting people know that this package is deprecated which the package manager is good at now. It’s minimally breaking and we can shift the docs in a clean way over to the toolkit

@aaronpowell
Copy link
Member

Wakes up, ready for another easy day to start the new year. What the notifications! 🤣

I'd be comfortable for the Community Toolkit to take over the ownership of the Dapr integration, with the expectation that:

  • There are people stepping up to maintain/evolve it (I'd assume that @FullStackChef would be interested). I've personally never used Dapr so I have zero skills on how to support it, beyond "that code looks ok"
  • Package name and namespaces (where applicable) are migrated to our naming conventions of CommunityToolkit prefix. Everything needs to be consistent in this space and I don't want anything to be treated as a special case because of where it might have come from (we've had other integrations that we've adopted from community members).

For deprecation of the existing package, it could take a dependency on the Community Toolkit package and mark the existing types/methods as Obsolete with IsError false for a release (lets pretend it's 9.1), while just acting as pass-through to the Community Toolkit types/methods. Then in the following Aspire release (9.2), the IsError becomes true (and drops the package dependency), forcing people to migrate across manually. Finally, the Aspire-owned package stops getting published to nuget at the 9.3 timeframe as it's removed from the Aspire repo. Also, for the whole timeframe the package is marked as deprecated on nuget, giving that additional layer of observation to people that they need to migrate across.

@davidfowl davidfowl changed the title [Feature] Dapr - Azure extensions + Redis configuration [Feature] Dapr - Migrate integration to the Community Toolkit Jan 2, 2025
@davidfowl
Copy link

I like it.

@FullStackChef
Copy link
Author

FullStackChef commented Jan 2, 2025

  • There are people stepping up to maintain/evolve it (I'd assume that @FullStackChef would be interested). I've personally never used Dapr so I have zero skills on how to support it, beyond "that code looks ok"

Nah I'm out 🤣 (JK)

Just to be clear. Prior to Aspire I had no knowledge of Dapr - I looked at it because I figured if there was an aspire integration it must be useful. I have found that to be correct and both the aspire + dapr communities to be super helpful. I've got enough coding experience and (probably more importantly) I'm stubborn enough to figure things out.

I've tried to replicate the approaches in other aspire integrations and I'm super keen to contribute but I don't claim to be an expert by any stretch and my experience is fully based in azure. That said if others like @paule96 are keen to contribute and there are people with knowledge to provide some direction then I'll happily do the work

@FullStackChef FullStackChef mentioned this issue Jan 6, 2025
10 tasks
@FullStackChef
Copy link
Author

@davidfowl & @aaronpowell Could I please get some feedback on #368

I've migrated Aspire.Hosting.Dapr however attempting to migrate unit tests is a rabbit warren that runs deep into internals of the Aspire.Hosting libraries

It would be good if I can get some guidance on how to approach this without rewriting aspire 😅

@davidfowl
Copy link

Why not do the lift and shift and the azure dapr extensions in different PRs? That'll make it easier to review. There's lots of new code to look at in the azure dapr extensions.

@FullStackChef
Copy link
Author

Why not do the lift and shift and the azure dapr extensions in different PRs? That'll make it easier to review. There's lots of new code to look at in the azure dapr extensions.

Why not indeed:

A) You changed the title of my issue and now it doesn't match the summary - this is my attempt at payback 🤣
B) That's just how I roll
C) There's some changes required for the dapr lift and shift that I'd already made and I don't like re-work
D) All of the above

Jokes aside, makes a lot of sense to separate these, I think we should :

  • Revert the title of this issue back to reflect the actual described integration
  • I can create a PR for this issue that has just the new integration (referencing the current dapr integration)
  • Create a separate issue for the lift and shift and link back to this issue for the comments
  • Make sure approach is covered off including unit tests / examples
  • If need be I can create a third issue to update the azure / redis integration to use the shifted version

This will be the least amount of re-work for me, and the azure / redis integration can be reviewed etc separately to the lift and shift

@davidfowl davidfowl changed the title [Feature] Dapr - Migrate integration to the Community Toolkit [Feature] Dapr - Azure extensions + Redis configuration Jan 6, 2025
@davidfowl
Copy link

Changes the title back. Let’s open a new issue with the migration.

@WhitWaldo
Copy link

WhitWaldo commented Jan 8, 2025

Hello all! I'm one of the Dapr .NET maintainers. Someone shared this thread with me yesterday and I thought I'd share my own thoughts here instead of just the Dapr Discord channel.

It's great to see that we've all found a common ground regarding the ongoing development and maintenance of the Aspire and Dapr integration. This addresses the concerns raised at the end of 2024:

  1. The Aspire team successfully offloads Dapr-specific development to an external group.
  2. The Dapr team avoids taking singular responsibility for another external project without any standing development commitment.
  3. Personally, I'm not aware of anything that prevents me or others from contributing PRs that add any particular features myself in the CommunityToolkit repo.

Once this has achieved some degree of maturity in this repository, there's also nothing that prevents us from revisiting this conversation about lifting and integrating a feature-complete tool out from CommunityToolkit and into the Dapr project for long-term maintenance.

Thank you to everyone involved for your collaboration and efforts in making this possible and please, don't hesitate to ping me on an issue here (@WhitWaldo), on the Dapr Discord channel (Whit), or send me a PM directly on Discord (username: Xaniff) if you need any assistance.

@philliphoff
Copy link

I will chime in too as a (former?) Dapr .NET maintainer and original developer of the Dapr & Aspire integration. I'm glad there is renewed interest in using Dapr with Aspire and that others seem willing to pick up that torch. As much as I love Dapr (and Aspire), my current work no longer affords me the opportunity to contribute in an official capacity and my list of "side projects" is already a mile long (and decidedly kid-focused at the moment--3D printing ideas, anyone?).

I'm more than willing to chat about the current implementation, possible future directions, etc., but, as much as I'd like to, it's unlikely that I can contribute myself (for the time being). I'm just happy if it finds a place and people willing to keep it moving forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting response Waiting for the author of the issue to provide more information or answer a question help wanted Extra attention is needed integration A new .NET Aspire integration
Projects
None yet
8 participants