Make username parameter always be injected. #4102
Conversation
dotnet-issue-labeler
bot
added
the
area-app-model
mitchdenny
added
the
breaking-change
| @@ -17,7 +17,7 @@ public class PostgresServerResource : ContainerResource, IResourceWithConnection | |||
| /// <param name="name">The name of the resource.</param> | |||
| /// <param name="userName">A parameter that contains the PostgreSQL server user name, or <see langword="null"/> to use a default value.</param> | |||
| /// <param name="password">A parameter that contains the PostgreSQL server password.</param> | |||
| public PostgresServerResource(string name, ParameterResource? userName, ParameterResource password) : base(name) | |||
| public PostgresServerResource(string name, ParameterResource userName, ParameterResource password) : base(name) | |||
There was a problem hiding this comment.
We should be consistent across all resources that take a user name.
| @@ -17,7 +17,7 @@ public class PostgresServerResource : ContainerResource, IResourceWithConnection | |||
| /// <param name="name">The name of the resource.</param> | |||
| /// <param name="userName">A parameter that contains the PostgreSQL server user name, or <see langword="null"/> to use a default value.</param> | |||
| @@ -34,7 +34,7 @@ public PostgresServerResource(string name, ParameterResource? userName, Paramete | |||
| /// <summary> | |||
| /// Gets the parameter that contains the PostgreSQL server user name. | |||
| /// </summary> | |||
| public ParameterResource? UserNameParameter { get; } | |||
| public ParameterResource UserNameParameter { get; } | |||
|
|
|||
| internal ReferenceExpression UserNameReference => | |||
There was a problem hiding this comment.
This can be removed now that the UserNameParameter can't be null.
| @@ -17,7 +17,7 @@ public class PostgresServerResource : ContainerResource, IResourceWithConnection | |||
| /// <param name="name">The name of the resource.</param> | |||
There was a problem hiding this comment.
Should be able to remove line 12:
private const string DefaultUserName = "postgres";
| "postgres-username": { | ||
| "type": "parameter.v0", | ||
| "value": "{postgres-username.inputs.value}", | ||
| "inputs": { | ||
| "value": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Does this mean that users will always be prompted for a user name when they deploy via azd?
There was a problem hiding this comment.
Is there a way to have the default value be a constant?
There was a problem hiding this comment.
I don't think that was ever implemented in azd, or Aspire.
The way we do it today is we just put the constant directly where it needs to go. i.e. it isn't a "parameter".
There was a problem hiding this comment.
What we could do is expand the the manifest so you can do this:
{
"inputs": {
"value": {
"type": "string",
"default": {
"value": "postgres"
}
}
}
}This would be a trigger for an aspire-8.1.json schema. If value is provided then generate isn't allowed. azd would prompt and show the default value (the user just hits ENTER). If it is secret it wouldn't show the value but indicate somehow that it should accept the default.
/cc @vhvb1989
There was a problem hiding this comment.
So do we have enough to decide what we want to go ahead with this? We would need:
- A change to AZD to support using default values if they are present (in accordance with the schema above).
- A change to our schema (introduce an 8.1 version of the schema)
- Introduce the necessary API changes to allow for setting the default value.
There was a problem hiding this comment.
Can we make this change in a non breaking way that doesn't require a schema update? AZD can prompt in older versions.
There was a problem hiding this comment.
Well the value field is additive, so we can add it without rev'ing the schema. And tools use use the value if they understand it. We might be able to get away with that?
There was a problem hiding this comment.
Added an API proposal with the manifest schema change above #4429
There was a problem hiding this comment.
I now think we shouldn't version the schema here. On an older azd you'll get the prompting behavior.
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Fixes #4100 and #4099
This PR does a few things:
UserNameParameteron the Postgres server resource.UserNameParameteron the postgres server resource to be non-nullable and adjust in the constructor as well.The reason for this change is that if you want to use the Postgres database resource from a container that is not built around .NET, the connection string format that the resource exposes won't necessarily be in the format that you want. For that reason you want to be able to get at the username and password parameters and know that they are always there. The password parameter is fine, but the fact that the username parameter is optional is problematic and makes consuming code more complex.
This represents a breaking change ... see unshipped API diff.
Microsoft Reviewers: Open in CodeFlow