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

AddParameter(...) #2005

Merged
merged 13 commits into from
Feb 2, 2024
Merged

AddParameter(...) #2005

merged 13 commits into from
Feb 2, 2024

Conversation

mitchdenny
Copy link
Member

@mitchdenny mitchdenny commented Jan 31, 2024

Fixes #1958

  • AddParameter method
  • Surrogate for connection strings (to give ParameterResource the IResourceWithConnectionStrings interface)
  • Manifest writing
  • AddConnectionString variant
  • Docs
  • Unit tests
Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jan 31, 2024
@mitchdenny mitchdenny self-assigned this Jan 31, 2024
@mitchdenny mitchdenny added this to the preview 4 (Mar) milestone Jan 31, 2024
@davidfowl
Copy link
Member

Also delete WithReference(ConnectionString)

@mitchdenny
Copy link
Member Author

Still a work in progress. Manifest does not presently generate correctly.

@mitchdenny
Copy link
Member Author

@davidfowl latest PR does some fairly major stuff in terms of changing the way connection strings work so that a context variable is passed in as well as there being a global execution context (this is the beginning of avoiding having to interpret the --publisher switch.

I started pulling a thread here and this is where I've ended up. It might end up being too big and I need to back out of it but I think it is worth exploring still.

@davidfowl
Copy link
Member

@davidfowl latest PR does some fairly major stuff in terms of changing the way connection strings work so that a context variable is passed in as well as there being a global execution context (this is the beginning of avoiding having to interpret the --publisher switch.

Why is that change required?

@mitchdenny
Copy link
Member Author

Why is that change required?

Gather around for an epic tale :)

Why DistributedApplicationExecutionContext?

In the issue for this PR we have the following code:

var parameter = builder.AddParameter("myconnectionstring", secret: true);
var catalogdb = builder.Operation switch {
    Publish => builder.AddParameter("catalogdb", secret: true).AsConnectionString(),
    Run => builder.AddPostgres("postgres").AddDatabase("catalogdb")
};

var myapp = builder.AddProject<Projects.MyApp>("myapp");
                   .WithReference(catalogdb);

The DistributedApplicationExecutionContext is a type that exposes an enumeration DistributedApplicationOperation. It is my approach to builder.Operation in the code above. So, it will be builder.ExecutionContext.Operation instead. My thinking is that we'll need a contextual object in multiple places and may want to expose various methods/properties on it.

Rather than defining multiple properties on the builder which disappears after we start running the DistributedApplicationExecutionContext is exposed on the builder, but also injected into the DI container.

Why string? GetConnectionString() to void ApplyConnectionString(ConnectionStringCallbackContext context)?

Consider the following code:

var parameter = builder.AddParameter("myconnectionstring", secret: true);
var catalogdb = builder.Environment.EnvironmentName switch {
    "Production" => builder.AddConnectionString("catalogdb"),
    "" => builder.AddPostgres("postgres").AddDatabase("catalogdb")
};

var myapp = builder.AddProject<Projects.MyApp>("myapp");
                   .WithReference(catalogdb);

Here we are using environment name to switch between using connection strings or creating a database. This means that the manifest that is generated will produce two different results depending on the environment. Here is an example of the JSON for each environment (simplified for the sake of brevity):

For Production:

{
  "resources": {
    "catalogdb": {
      "type": "parameter.v0"
      "value": "{catalogdb.inputs.value}",
      "inputs": {
        "value": {
          "type": "string",
          "secret": true
        }
      }
    }
    "myapp": {
      "type": "project.v0"
      "env": {
        "ConnectionStrings_catalogdb": "{catalogdb.value}"
      }
    }
  }
}

For everything else:

{
  "resources": {
    "postgres": {
      "type": "postgres.server.v0"
    },
    "catalogdb": {
      "type": "postgres.database.v0",
      "parent": "postgres"
    }
    "myapp": {
      "type": "project.v0"
      "env": {
        "ConnectionStrings_catalogdb": "{catalogdb.connectionString}"
      }
    }
  }
}

The key issue is the difference in these two lines:

        "ConnectionStrings_catalogdb": "{catalogdb.value}"
        "ConnectionStrings_catalogdb": "{catalogdb.connectionString}"

In the code on main the resource types themselves don't emit the {catalogdb.connectionString} value. Instead it is the WithReference(...) for connection string resources that implement this type. When dealing with a parameter which has a connection string surrogate wrapped around it, using {catalogdb.connectionString} would be invalid.

We could put in some special case handling for the surrogate and not have to make these changes. But it also sounded like we might need to accommodate connection strings becoming connection properties - if we head down that path then we'll probably need to have some kind of context object arrangement like this. We may also need to go async to support provisioning scenario as well (not sure on that yet).

The way it works is that the ExecutionContext is exposed as a property on the ConnectionStringCallbackContext so these methods now have much greater access, they may also possibly be able to resolve services from DI, although we don't presently do that.

Current challenges

I'm currently looking at how the Azure provisioner works in context of all this but I think it'll work OK.

@mitchdenny
Copy link
Member Author

... the other thing I am toying with is getting rid of the UseEmulator(...) calls for Azure Storage and Cosmos and instead making them the default behavior but that requires a deeper conversation.

@mitchdenny mitchdenny force-pushed the mitchdenny/add-parameter-take-two branch from d149992 to 07234d2 Compare February 2, 2024 00:44
@mitchdenny
Copy link
Member Author

Reverted to an earlier commit. Exploration of this idea is on this branch where we can refer back to it if/when we want to tackle this issue:

https://github.com/dotnet/aspire/tree/mitchdenny/apply-connectionstring-exploration

@mitchdenny mitchdenny marked this pull request as ready for review February 2, 2024 03:03
@mitchdenny mitchdenny enabled auto-merge (squash) February 2, 2024 06:41
@mitchdenny mitchdenny merged commit 9da099a into main Feb 2, 2024
8 checks passed
@mitchdenny mitchdenny deleted the mitchdenny/add-parameter-take-two branch February 2, 2024 07:15
radical pushed a commit to radical/aspire that referenced this pull request Feb 6, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Apr 22, 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.

AddParameter support
2 participants