Add additional/change `WithImage` extension method for PostgreSQL

[MadL1me]

In many cases of using docker, we usually specify image + tag in one continious string in a following format: "{image}:{tag}", for example like postgres:14.3. This notation is used with direct docker command, as well as on dockerhub or in Dockerfiles and Docker-compose files, and it might be a little bit better to be able just to copy one string and specify it in extension method.

But current extension method specifies tag = "latest" directly, so you can't just set "postgres:14.3" in image. For me that was a surprise, and I've got runtime error:

fail: Aspire.Hosting.Dcp.dcpctrl.ContainerReconciler[0]
      could not create the container    {"Container": {"name":"database"}, "Reconciliation": 3, "error": "CreateContainer command returned error code 1 Stdout: '' Stderr: 'invalid reference format\n'"}

In my opinion, change of extension method will add a little bit to a better developer experience:

Before:

var builder = DistributedApplication.CreateBuilder(args);

builder.AddPostgres("database")
    .WithImage("postgres", "16.2")

builder.Build().Run();

After:

var builder = DistributedApplication.CreateBuilder(args);

builder.AddPostgres("database")
    .WithImage("postgres:16.2")
    .WithPgAdmin();

builder.Build().Run();

[mitchdenny]

If you are only changing the tag couldn't you just use WithImageTag?

[davidfowl]

I think we should support this. We'd be willing to take a PR if there was a sane way to parse the registry, image name and tag. @danegsta do you know if there's a spec? Or is this just docker magic?

[danegsta]

I’ve never found a formal spec, but the implementations for image format are consistent and stable between Docker and Podman, so we were able to put together a regex that handles all image tag parts in the vscode-docker extension: https://github.com/microsoft/vscode-docker-extensibility/blob/759aefe83ed6af599db22fd6c4db963ff40c6323/packages/vscode-container-client/src/utils/parseDockerLikeImageName.ts#L22

Should be pretty straightforward to repurpose that same regex/logic for Aspire.

[robertcoltheart]

I'll also comment here that there are valid registries eg. artifactory that require you to pass a port as part of the image name.

For example:

.WithImage("artifactory.my.org:9956/redis", "7.2.4") or .WithImage("artifactory.my.org:9956/redis")

both fail with the above error.

The workaround is to use .WithImageRegistry("artifactory.my.org:9956") however the WithImage syntax runtime error came as a surprise, much like the OP.

[davidfowl]

Marking this as a good first issue that we'll take a contribution for. The work looks like taking that regex and setting the right state based on the capture groups.

[afscrome]

This seems like similar to #4218 - same underlying issue, although with different entrypoints (AddContainer vs WithImage)

[MadL1me]

@davidfowl I'll take the issue to contribute

Is the flow similar to ASP.NET Core contributions? Do we need to decide on the API signatures before moving forward? Or I can just jump straight into PR? (cause it's pretty straighforward - just add an additional method with just only one string param)

Also, @afscrome targeted similar thing, I think we should change this for other similar functions as well

[davidfowl]

Is the flow similar to ASP.NET Core contributions? Do we need to decide on the API signatures before moving forward? Or I can just jump straight into PR? (cause it's pretty straighforward - just add an additional method with just only one string param)

Do we need a method? I think WithImage just parses the string and sets the other properties if it can.

[MadL1me]

Do we need a method? I think WithImage just parses the string and sets the other properties if it can.

The current signature looks like this:

IResourceBuilder<T> WithImage<T>(this IResourceBuilder<T> builder, string image, string tag = "latest") where T : ContainerResource

thing I don't like, is that, from user perspective it is not very intuitive - even if you passed image with correct format, the default second param for image tag still would be "latest". This is why I though about second method, but without tag param at all

[davidfowl]

Can you update the issue with the new API using this template

[afscrome]

Here's a stab with the API Template

Background and Motivation

Most places in the container ecosystem treat the image reference as a single string. For example, the docker run command:

docker run redis:7.2.5

Or a Kubernetes container spec:

apiVersion: v1
kind: Pod
metadata:
  name: sql
spec:
  containers:
  - name: sql
    image: mcr.microsoft.com/mssql/server:2022-latest

However if you try that in Aspire as follows:

IDistributedApplicationBuilder.AddContainer("mcr.microsoft.com/mssql/server:2022-latest");
IResourceBuilder<T>.WithImage("mcr.microsoft.com/mssql/server:2022-latest")

Not only does it fail, but it fails with an error quite a distance away from where you provided the bad image, and with an error that's not immediately clear why the reference is invalid

fail: Aspire.Hosting.Dcp.dcpctrl.ContainerReconciler[0] could not create the container {"Container": {"name":"database"}, "Reconciliation": 3, "error": "CreateContainer command returned error code 1 Stdout: '' Stderr: 'invalid reference format\n'"}

Aspire instead expects you to do the following

IDistributedApplicationBuilder
    .AddContainer("mssql/server", "2022-latest")
    .WithImageRegistry("mcr.microsoft.com")

Proposed API

ContainerResourceBuilderExtensions.WithImage

We should accept users passing a full image reference to WithImage, and break it out into the constituent parts,

namespace Aspire.Hosting;
public static class ContainerResourceBuilderExtensions
{
-        public static IResourceBuilder<T> WithImage<T>(this IResourceBuilder<T> builder, string image, string tag = "latest") where T : ContainerResource
+        public static IResourceBuilder<T> WithImage<T>(this IResourceBuilder<T> builder, string image) where T : ContainerResource
+        public static IResourceBuilder<T> WithImage<T>(this IResourceBuilder<T> builder, string image, string tag) where T : ContainerResource
}

The WithImage(string) method parse the input string, break it up into registry, image, tag (or sha256), and then use those to build up the ContainerImageAnnotation added to the resource. Using the rules described at https://github.com/dotnet/aspire/issues/4355#issuecomment-2147862676

What the WithImage(string,string) does has a couple of wrinkles - in particular what happens with the following

builder.WithImage("redis:7.2.5", "latest")
builder.WithImage("redis@sha256:f5ef9e24a9ef3b7cc552ae0cbc3cbade4f2877502683496c5d775605ae071412", "7.2.5")

Those overload are ambiguous as to what image should be referenced. My first thought of a solution here would be to limit the parsing of the image reference to WithImage(string), and leave ContainerResourceBuilderExtensions.WithImage(string,string) behaving exactly as it does today. However that then means the following would continue to fail with a cryptic error, even though we could resolve that unambiguously.

.WithImage("mcr.microsoft.com/mssql/server", "2022-latest")

That leaves us with two options

1. Parse the image as an image reference, and throw an error if there's ambiguity due to two tags, or a tag and a digest
2. Do no parsing of the image, and fail later on if an image reference is passed

I'm inclined to go for option 1, although that's not a particularly strongly held view.

IDistributedApplicationBuilder.AddContainer

No API signature changes should be required, however the AddContainer(string) and AddContainer(string,string) methods should follow the same rules as described above for WithImage.

This shouldn't be too problematic as AddContainer ultimately calls WithImage, but a change may be needed to how the calls chain together. Currently it goes AddContainer(string) --> AddContainer(string,string) --> WithImage(string,string), but that may need to become AddContainer(string) --> WithContainer(string)

Usage Examples

The following will continue to behave exactly as it does today

builder.WithImage("redis")

The following fail today, but after this proposal should work as if an equalivant chain of WithImage, WithImageRegistry and/or WithImageSHA256 calls were used:

// image & tag
builder.WithImage("redis:latest")
builder.WithImage("redis:7.2.5")

// image & digest
builder.WithImage("redis@sha256:f5ef9e24a9ef3b7cc552ae0cbc3cbade4f2877502683496c5d775605ae071412")

// registry & image
builder.WithImage("quay.io/prometheus/prometheus")

// registry, image & tag
builder.WithImage("mcr.microsoft.com/mssql/server:2022-latest")
builder.WithImage("localhost:1235/mcr.microsoft.com/mssql/server:2022-latest")

// registry, image & digest
builder.WithImage("mcr.microsoft.com/mssql/server@sha256:c4369c38385eba011c10906dc8892425831275bb035d5ce69656da8e29de50d8")

For each of the above examples, IDistributedApplicationBuilder.AddContainer should behave the same as th equilivant IResourceBuilder<T>.WithImage method

Alternative Designs

Handle Errors better

One solution to this problem is to simply improve the errors. Rather than failing later on with a cryptic error, far away from where the image was provided, we could fail much earlier. E.g fail in WithImage so the debugger breaks on the line of code where the user provided the image reference with an error along the lines of:

Image references not supported - break your image reference up into separate image and tag fields in addition to using WithContainerRegistry for any registry components.

However that doesn't feel like a great user experience, especially since there's a good chance that adding a container reference is one of the first things people do with aspire, presenting a bad first impression.

Analysers & Code Fixes

We could build some analysers and code fixes to detect this cases, but that feels like a large amount of work just to provide a worse developer experience. There are also likely to be many edge cases that our analysers don't cover. (e.g. when the image reference is built up dynamically)

Risks

Making an optional parameter no longer optional is a breaking change, however I believe the addition of the single string overload would get resolved in all source breaking changes that would have previously broken making this an acceptable change. If that's not the case, the string tag parameter could remain as an optional parameter.