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

Update TestShop to store RabbitMQ password in user secrets #3446

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

DamianEdwards
Copy link
Member

@DamianEdwards DamianEdwards commented Apr 5, 2024

This updates the TestShop app to automatically store the generated password for RabbitMQ in user secrets so that it runs after cloning without any further action from the developer.

This is a pattern we'll likely want to productize and have the relevant resources support (perhaps by default) as tracked in #1151

Microsoft Reviewers: Open in CodeFlow

@DamianEdwards DamianEdwards requested a review from eerhardt April 5, 2024 23:36
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Apr 5, 2024
@DamianEdwards DamianEdwards changed the title Update TestShop to store RabbitMQ in user secrets Update TestShop to store RabbitMQ password in user secrets Apr 5, 2024

public override void WriteToManifest(ManifestPublishingContext context) => parameterDefault.WriteToManifest(context);

private static bool TrySetUserSecret(string applicationName, string name, string value)
Copy link
Member

Choose a reason for hiding this comment

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

This is quite different to the way we handle user secrets in the Azure Provisioner (for example) where we load the secrets.json file directly and then save it. There are some quirks here in the way that dotnet user-secrets injects values into the secrets file. Parameters:secret name will be a flatted value at the root of the document.

Copy link
Member

Choose a reason for hiding this comment

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

That said I'm supportive of doing something like this because it reduces friction when you clone the repo and hit F5.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I wouldn't suggest we productize this approach and was unaware we already had code in the repo to deal with saving user secrets. I ported this from what I'm doing in the samples repo. dotnet user-secrets is the only "official" code implementation we have that saves to the secret store today so this seemed like the easiest approach for now in the playground app until we implement this idea for real.

@eerhardt eerhardt requested a review from davidfowl April 9, 2024 16:25

if (builder.Environment.IsDevelopment() && builder.ExecutionContext.IsRunMode)
{
// In development mode, generate a new password each time the application starts
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this comment makes sense. We want to get it from user secrets if it is there in development mode.

bool lower = true, bool upper = true, bool numeric = true, bool special = true,
int minLower = 0, int minUpper = 0, int minNumeric = 0, int minSpecial = 0)
{
ParameterDefault generatedPassword = new GenerateParameterDefault
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ParameterDefault generatedPassword = new GenerateParameterDefault
ParameterDefault parameterDefault = new GenerateParameterDefault

Since sometimes this isn't a "generatedPassword" since it could come from user secrets.

}
}

sealed class ResourceBuilder
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a problem with our Hosting code that you need to make these classes?


public override void WriteToManifest(ManifestPublishingContext context) => parameterDefault.WriteToManifest(context);

private static bool TrySetUserSecret(string applicationName, string name, string value)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private static bool TrySetUserSecret(string applicationName, string name, string value)
private static void SetUserSecret(string applicationName, string name, string value)

Nothing uses this bool.

@DamianEdwards DamianEdwards merged commit 5200f67 into main Apr 12, 2024
8 checks passed
@DamianEdwards DamianEdwards deleted the damianedwards/TestShop-stable-passwords branch April 12, 2024 02:12
@github-actions github-actions bot locked and limited conversation to collaborators May 12, 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.

3 participants