-
Notifications
You must be signed in to change notification settings - Fork 134
Add flagd Aspire hosting #827
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
Add flagd Aspire hosting #827
Conversation
c671d0f to
b3cefbc
Compare
| return builder.AddResource(resource) | ||
| .WithImage(FlagdContainerImageTags.Image, FlagdContainerImageTags.Tag) | ||
| .WithImageRegistry(FlagdContainerImageTags.Registry) | ||
| .WithHttpEndpoint(port: 8013, targetPort: port, name: FlagdResource.HttpEndpointName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: create a constant FlagdPort
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with f468d42
| this IDistributedApplicationBuilder builder, | ||
| [ResourceName] string name, | ||
| string fileSource, | ||
| int port = 8013) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: the other integration uses int? port = null so that it provides a dynamic port by default, allowing the possibility to override the port only when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, should be defaulting to null and leave that to Aspire to assign a port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with 6317cd6
| public static IResourceBuilder<FlagdResource> AddFlagd( | ||
| this IDistributedApplicationBuilder builder, | ||
| [ResourceName] string name, | ||
| string fileSource, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: if you expect a default value for this, why not set it here?
| string fileSource, | |
| string fileSource = "flagd.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still thinking on what to do here. The main reason is flagd supports multiple sources and at this moment I am limiting just to file system. Thinking on if I should add a minimal working hosting that works with just files or adding now all the possible syncs. Check: https://flagd.dev/concepts/syncs/
| /// <param name="builder">The resource builder.</param> | ||
| /// <param name="level">The logging level (debug, info, warn, error).</param> | ||
| /// <returns>The <see cref="IResourceBuilder{FlagdResource}"/>.</returns> | ||
| public static IResourceBuilder<FlagdResource> WithLogging( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: It may be me but I would have expect WithLogLevel(builder, FlagdLogLevel level) with FlagdLogLevel being a specifc enum of possible values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc. @aaronpowell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, an enum would be better as that is more discoverable to the end user.
Alternatively, shouldn't it just inherit the logging level from the standard logger? Is there a reason you'd configure flagd to log a different verbosity level to the whole app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what the default logger would be. If it's like goff, the default log level is INFO. You can still override it within the config file and if needed by the environment variable. I do think that overriding with an env variable like this is the best solution. What did you have in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with a03c6e0
This was still code from the initial scaffolding the copilot did when I used the agent in this repo to create the structure for this hosting project. Now it is fixed to add extra logging when a flag is evaluated: https://flagd.dev/troubleshooting/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I checked and I don't see any possibility to set the log level (like warning or info). It's either DEBUG or not.
I would go for a more generic solution/naming that can be reused for other Hosting integration like goff, I suggest this:
enum FlagdLogLevel
{
Error = 0,
Debug
}
public static IResourceBuilder<FlagdResource> WithLoglevel(
this IResourceBuilder<FlagdResource> builder,
FlagdLogLevel logLevel)
{
// ..
return builder.WithEnvironment("FLAGD_DEBUG", logLevel == FlagdLogLevel.Error ? "true" : "false");
}Even better would be reusing this https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.logging.loglevel?view=net-9.0-pp
public static IResourceBuilder<FlagdResource> WithLoglevel(
this IResourceBuilder<FlagdResource> builder,
LogLevel logLevel)
{
// ..
if (logLevel == LogLevel.Debug)
{
return builder.WithEnvironment("FLAGD_DEBUG", "true");
}
else if (logLevel == LogLevel.Error)
{
return builder;
}
else
{
// throw error as not expected?
}
}What do you think @aaronpowell ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It definitely gives a more .NET-like look and feel. Waiting on feedback to make that change since it feels a design we might want to replicate in other integrations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let's go idiomatic with the .NET logging levels. I think that raising an exception if someone uses a log level that's unsupported would be the right way to go as that avoids a false sense of what logging can achieve, and allows for flexibility if flagd adds additional logging levels in the future.
| builder.Eventing.Subscribe<AfterResourcesCreatedEvent>(static async (@event, cancellationToken) => | ||
| { | ||
| // When the resources are created, set the OpenFeature provider to use Flagd (debug purposes) | ||
| await OpenFeature.Api.Instance.SetProviderAsync(new FlagdProvider()); | ||
|
|
||
| var flagClient = OpenFeature.Api.Instance.GetClient(); | ||
| var welcomeBanner = await flagClient.GetBooleanDetailsAsync("welcome-banner", false); | ||
| var backgroundColor = await flagClient.GetStringDetailsAsync("background-color", "000000"); | ||
| var apiVersion = await flagClient.GetStringDetailsAsync("api-version", "0.1"); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this for illustrating something specific in the example apphost, or is this the kind of thing that we could roll up into an extension method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaronpowell I am using this as a testing project to see if the integration is working properly. With this I can test a flag evaluation when I start this aspire project.
Ideally, I will add later this Service extension: #800, which will deal with the DI and so on. Similar to what I have here: https://github.com/askpt/openfeature-aspire-welcome/blob/550eb169399da489916ebf9360503df99cf9befe/Todo.ServiceDefaults/Extensions.cs#L32
| this IDistributedApplicationBuilder builder, | ||
| [ResourceName] string name, | ||
| string fileSource, | ||
| int port = 8013) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, should be defaulting to null and leave that to Aspire to assign a port.
| /// <param name="builder">The resource builder.</param> | ||
| /// <param name="level">The logging level (debug, info, warn, error).</param> | ||
| /// <returns>The <see cref="IResourceBuilder{FlagdResource}"/>.</returns> | ||
| public static IResourceBuilder<FlagdResource> WithLogging( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, an enum would be better as that is more discoverable to the end user.
Alternatively, shouldn't it just inherit the logging level from the standard logger? Is there a reason you'd configure flagd to log a different verbosity level to the whole app?
| public async Task ResourceStartsAndRespondsCorrectly() | ||
| { | ||
| var resourceName = "flagd"; | ||
| await fixture.ResourceNotificationService.WaitForResourceAsync(resourceName).WaitAsync(TimeSpan.FromMinutes(1)); | ||
| var httpClient = fixture.CreateHttpClient(resourceName); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything we can assert against on the resource to verify that it's running beyond "it didn't time out"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still implementing this tests and cleanup some code. I am planning on adding a flag evaluation here, similar to what is done in the examples project.
fde75aa to
3fa7e87
Compare
c0171e6 to
85f6924
Compare
|
@aaronpowell could you please reopen this PR? I am still working on it |
85f6924 to
0dc4471
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new .NET Aspire hosting integration for flagd, a feature flag evaluation engine that provides an OpenFeature-compliant backend system. The integration allows developers to easily add flagd containers to their Aspire applications with support for flag configuration files.
Key changes include:
- Complete flagd hosting integration with resource definition, builder extensions, and configuration options
- Comprehensive test coverage for the new integration
- Example application demonstrating flagd usage
- Documentation and project configuration updates
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/CommunityToolkit.Aspire.Hosting.Flagd/FlagdResource.cs |
Defines the FlagdResource class with endpoint references and connection string implementation |
src/CommunityToolkit.Aspire.Hosting.Flagd/FlagdBuilderExtensions.cs |
Provides extension methods for adding flagd to applications with configuration options |
src/CommunityToolkit.Aspire.Hosting.Flagd/FlagdContainerImageTags.cs |
Constants for flagd container image registry, name, and version |
tests/CommunityToolkit.Aspire.Hosting.Flagd.Tests/AddFlagdTests.cs |
Unit tests covering all builder extension methods and resource configuration |
tests/CommunityToolkit.Aspire.Hosting.Flagd.Tests/AppHostTests.cs |
Integration tests verifying flagd functionality with both flagd and OFREP providers |
examples/flagd/CommunityToolkit.Aspire.Hosting.Flagd.AppHost/Program.cs |
Example application showing how to configure flagd with file sync and logging |
examples/flagd/CommunityToolkit.Aspire.Hosting.Flagd.AppHost/flags/flagd.json |
Sample flag configuration file for testing and demonstration |
README.md |
Updated main README to include flagd integration in the toolkit listing |
aaronpowell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once there's the update with the logging design, I think this will be ready to merge in
| /// <param name="builder">The resource builder.</param> | ||
| /// <param name="level">The logging level (debug, info, warn, error).</param> | ||
| /// <returns>The <see cref="IResourceBuilder{FlagdResource}"/>.</returns> | ||
| public static IResourceBuilder<FlagdResource> WithLogging( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let's go idiomatic with the .NET logging levels. I think that raising an exception if someone uses a log level that's unsupported would be the right way to go as that avoids a false sense of what logging can achieve, and allows for flexibility if flagd adds additional logging levels in the future.
|
@askpt can you add the test project to the list so that the tests are run. |
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
…ge examples Signed-off-by: André Silva <[email protected]>
… related tests Signed-off-by: André Silva <[email protected]>
…oint Signed-off-by: André Silva <[email protected]>
…ing level and update related tests Signed-off-by: André Silva <[email protected]>
Signed-off-by: GitHub <[email protected]>
Signed-off-by: GitHub <[email protected]>
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
…ource configuration Signed-off-by: André Silva <[email protected]>
…d endpoint validation Signed-off-by: André Silva <[email protected]>
…urations Signed-off-by: André Silva <[email protected]>
… removing skip attribute Signed-off-by: André Silva <[email protected]>
…ync method Signed-off-by: André Silva <[email protected]>
Co-authored-by: Copilot <[email protected]>
Updated README to clarify logging configuration and port customization for flagd.
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
…dition and error handling Signed-off-by: André Silva <[email protected]>
…ogLevel for improved clarity and functionality Signed-off-by: André Silva <[email protected]>
d74d47a to
a794065
Compare
@aaronpowell I fixed the test list and the log level suggestion. Should be now good to go 👍 |
Closes #737
PR Checklist
Other information