Consider removing provider.Context in favor of context.Context

[blampe]

Problem

https://github.com/golang/go/wiki/CodeReviewComments#contexts

Don't create custom Context types or use interfaces other than Context in function signatures.

If you have application data to pass around, put it in a parameter, in the receiver, in globals, or, if it truly belongs there, in a Context value.

I wish this guidance went into more detail around why to avoid custom Context types, but at least in our case the custom provider.Context type makes it difficult (impossible?) to unit test CRUD operations.

Specifically, if a CRUD operation needs access to the provider's configuration it must use infer.GetConfig to get it. GetConfig requires a provider.Context from a Wrap'd provider, but once you have that provider you can't get a provider.Context from it because ctx is private. Instead, you need a server available (ResourceProviderServer or integration.Server) to generate a usable context.

pulumi-go-provider/infer/provider.go

Lines 129 to 134 in ab33426

	func GetConfig[T any](ctx p.Context) T {
	v := ctx.Value(configKey)
	var t T
	if v == nil {
	panic(fmt.Sprintf("Config[%T] called on a provider without a config", t))
	}

pulumi-go-provider/infer/provider.go

Lines 118 to 120 in ab33426

	provider = mContext.Wrap(provider, func(ctx p.Context) p.Context {
	return p.CtxWithValue(ctx, configKey, opts.Config)
	})

pulumi-go-provider/provider.go

Lines 587 to 589 in ab33426

	func (p *provider) ctx(ctx context.Context, urn presource.URN) Context {
	return &pkgContext{ctx, p, urn}
	}

In other words, as a user I want to be be able to unit test this function:

func (*Resource) Delete(
    ctx provider.Context, 
    id string,
    state ResourceState, 
) error {
    cfg := infer.GetConfig[Config](ctx)
    ...
}

but I require a server to produce a valid provider.Context, so I'm actually only able to test at a higher layer:

  err = server.Delete(provider.DeleteRequest{
      ID: "name",
      Urn: ...,
      Properties: resource.PropertyMap{...},
  })
  assert.NoError(t, err)

This is unfortunate because we lose one of the primary benefits of this library, namely the ability to work with simple structs instead of the lower-level resource.PropertyMap.

Solution

One solution would be to export provider.ctx(...) → provider.Ctx(...) so users can create a provider.Context more easily. This is an easy fix, but it still requires me to have a provider available, with configuration inferred, and so on. In other words, the framework is still "in the way," even for the simplest of test cases.

Instead, I would strongly recommend an alternative API that uses the standard library's context.Context. Consider some CRUD signatures with context.Context instead:

func (r *Resource) Create(
    ctx context.Context, 
    id string,
    args ResourceArgs,
    preview bool 
) (string, ResourceState, error) { ... }

func (r *Resource) Delete(
    ctx context.Context, 
    id string,
    state ResourceState, 
) error { ... }

We can immediately see how to unit test these because it's just a matter of instantiating a context.Background(), a &Resource{}, some inputs, and expected outputs.

But what to do about the methods currently on provider.Context? The Log* and RuntimeInformation methods could instead be made context-aware and embedded in the resource controller:

// User's controller
type Resource struct {
    *provider.Controller[Config]
}

// pulumi-go-provider
type Controller struct {}

func (c *Controller) Log(ctx context.Context, severity diag.Severity, msg string) {
    if c == nil {
        // No-op if unset in tests.
        return
    }
    ...
}

We can do this in such a way that the user doesn't need to instantiate anything they don't require, so the framework can get completely out of the way:

    r := &Resource{}
    err := r.Delete(context.Background(), "foo", ResourceState{ ... })
    assert.NoError(t, err)

We can also leverage this Controller concept to make it easier to grab the provider's configuration. Instead of cfg := infer.GetConfig[Config](ctx) we could have cfg := c.Controller.ProviderConfiguration().

Similarly, we could expose things like any options attached to the resource:

func (c *Controller) ResourceOptions() ResourceOptions { ... }

(AFAICT these controllers are scoped to individual resources, and we don't currently expose things like retainOnDelete.)

Taken all together, a more complex unit test could look something like this:

    r := &Resource{
        Controller: provider.NewController(
            provider.WithConfig(
                Config{
                    Foo: "bar",
                    client: mockClient,
                },
            ),
            provider.WithLogger(l),
            provider.WithRetainOnDelete(),
        )
    }
    err := r.Delete(context.Background(), "foo", ResourceState{ ... })
    assert.NoError(t, err)

This gives us the flexibility to easily override as little or as much as we need, without needing to deal with servers or test harnesses. If we want to test log output, we can inject a test logger. If we want to inject a mock client, we can inject that with the provider's configuration. Etc.

This would be a backwards-incompatible change, but the library currently makes no stability guarantees so this is OK. Updating an existing provider to use the new API would be a mechanical change.