Remove p.Context in favor of context.Context

[iwahbe]

Fixes #159

This change comes in two parts:

• 8abb083: Define a new Logger, accessed via p.GetLogger(ctx). Usage looks like this

  p.GetLogger(ctx).Errorf("Something has gone wrong: %s", opt.Error())

  The new p.GetLogger function has two "sink"s: a hostSink that equates to calling logging methods on the server host (which nicely displays to the user) and a fallback slogSink which uses log/slog to output structured error messages. The fallback sink ensures that p.GetLogger(context.Background()).Debug("Constructing Config") will still log. This should be useful only when testing.
• 6ffd425 switches over from p.Context to context.Context. The change is entirely mechanical.

Migration Guide (to be included in the release that goes out with this PR):

• Replace all instances of "github.com/pulumi/pulumi-go-provider".Context with
  context.Context.
• Methods on a ctx "github.com/pulumi/pulumi-go-provider".Context have become
  free-standing functions on "github.com/pulumi/pulumi-go-provider"
  • ctx.RuntimeInformation has become
    "github.com/pulumi/pulumi-go-provider".GetRunInfo(ctx).
  • ctx.Log* has been replaced by
    "github.com/pulumi/pulumi-go-provider".GetLogger(ctx).*.
  • "github.com/pulumi/pulumi-go-provider/infer".CtxFromPulumiContext(ctx) has been
    replaced by ctx.Context().

Design considerations

I decided on using a plain context.Context with freestanding functions wrapping context.Context.Value to keep usage idiomatic to Go and to allow arbitrary middleware to extend the values hanging on context.Context.

Why not a resource controller as recommended in #159?

A centralized resource controller doesn't decompose well between the main package and the infer package. Consider the example given in the issue (adjusted for provider -> infer):

    r := &Resource{
        Controller: infer.NewController( // <- Nit 1
            infer.WithConfig(
                Config{
                    Foo: "bar",
                    client: mockClient,
                },
            ),
            infer.WithLogger(l), // <- Problem 2
            infer.WithRetainOnDelete(), // <- Problem 3
        )
    }
    err := r.Delete(context.Background(), "foo", ResourceState{ ... })
    assert.NoError(t, err)

Nits & Problems:

1. The user should never call infer.NewController except in testing, so I would much rather see test functions in their own package: testinfer.NewController.
2. During normal runtime, we only want users to be using the logger that targets the plugin host, so I'm skeptical we need an extensible logging interface here. I see room for at most 3 interfaces:
   • The runtime interface used to display to the user.
   • A fallback interface to log to stdout.
   • A testing capture interface to allow users to check they log correctly (since logging is part of a providers UI)
3. Resource options are in general not passed to the provider. RetainOnDelete simply means the Delete method will be skipped. Pulumi providers never see it.

I'd prefer a design that looks like this:

    ctx := context.Background()

    // If not called, no config will be found and GetConfig will panic.
    //
    // If the resource does not use a custom configuration, then this is optional.
    ctx = infertest.InjectConfig(ctx, Config{
        Foo: "bar",
        client: mockClient,
    })

    // If not called, integration.GetLogOutput will panic.
    // 
    // Will be automatically called by `integration.NewServer`.
    // If logs do not need to be captured, then this is optional.
    ctx = integration.InjectTestLogger(ctx)

    // If not called, `p.GetRunInfo` will panic.
    //
    // Will be automatically called by `integration.NewServer`.
    // If `p.GetRunInfo` does not need to be called, this is optional.
    ctx = integration.InjectRunInfo(ctx, p.RunInfo{
        ProviderName: "some-provider",
        Version: "1.2.3",
    })

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

    validateLogs(integration.GetLogOutput(ctx))

This way all packages can extend the context in the same way.

---

I'm sorry to break existing users but I think this design is strictly better in the long term. Go has standardized on a context.Context type and the more Go's ecosystem gets behind it the easier everyone's life will be.

[guineveresaenger]

Would like a little bit more clarity on how this code discovers which logger to use but this seems like a good change.

The only question I'd have is how many users would be affected.

[guineveresaenger]

Can we add a few lines how this package helps us figure out what is available in our context?

[iwahbe]

I can add a few lines detailing why the package exists:

// key provides an internal set of keys for use with [context.WithValue] and
// [context.Context.Value] that can be shared across packages source.
//
// Each key has a private type (a `struct{}`) and a public instance of that type.

Can we add a few lines how this package helps us figure out what is available in our context?

keys doesn't help us figure out what is available in our context. It exists purely so we can share keys between packages in pulumi-go-provider without leaking that implementation to our users.

[blampe]

Below is a patch to automate most of the migration (note CtxFromPulumiContext isn't covered -- exercise for the reader 😉). This was sufficient to migrate my provider (pulumi/pulumi-docker-build#18) without any build errors (tests will need some cleanup since I'm using some mocks to work around p.Context).

The patch can be applied like so:

go run github.com/uber-go/gopatch@latest -p goprovider.patch ./provider/...

@@
var goprovider identifier
@@
 import goprovider "github.com/pulumi/pulumi-go-provider"
+import "context"

-goprovider.Context
+context.Context

@@
var goprovider identifier
@@
 import goprovider "github.com/pulumi/pulumi-go-provider"

-ctx.RuntimeInformation()
+goprovider.GetRunInfo(ctx)

@@
var goprovider identifier
var diag identifier
var ctx identifier
@@
 import diag "github.com/pulumi/pulumi/sdk/v3/go/common/diag"
 import goprovider "github.com/pulumi/pulumi-go-provider"

-ctx.Log(diag.Info, ...)
+goprovider.GetLogger(ctx).Info(...)

@@
var goprovider identifier
var diag identifier
var ctx identifier
@@
 import diag "github.com/pulumi/pulumi/sdk/v3/go/common/diag"
 import goprovider "github.com/pulumi/pulumi-go-provider"

-ctx.Logf(diag.Info, ...)
+goprovider.GetLogger(ctx).Infof(...)

@@
var goprovider identifier
var diag identifier
var ctx identifier
@@
 import diag "github.com/pulumi/pulumi/sdk/v3/go/common/diag"
 import goprovider "github.com/pulumi/pulumi-go-provider"

-ctx.LogStatus(diag.Info, ...)
+goprovider.GetLogger(ctx).InfoStatus(...)

@@
var goprovider identifier
var diag identifier
var ctx identifier
@@
 import diag "github.com/pulumi/pulumi/sdk/v3/go/common/diag"
 import goprovider "github.com/pulumi/pulumi-go-provider"

-ctx.LogStatusf(diag.Info, ...)
+goprovider.GetLogger(ctx).InfoStatusf(...)

@@
var goprovider identifier
var diag identifier
var ctx identifier
@@
 import diag "github.com/pulumi/pulumi/sdk/v3/go/common/diag"
 import goprovider "github.com/pulumi/pulumi-go-provider"

-ctx.Log(diag.Error, ...)
+goprovider.GetLogger(ctx).Error(...)

@@
var goprovider identifier
var diag identifier
var ctx identifier
@@
 import diag "github.com/pulumi/pulumi/sdk/v3/go/common/diag"
 import goprovider "github.com/pulumi/pulumi-go-provider"

-ctx.Logf(diag.Error, ...)
+goprovider.GetLogger(ctx).Errorf(...)

@@
var goprovider identifier
var diag identifier
var ctx identifier
@@
 import diag "github.com/pulumi/pulumi/sdk/v3/go/common/diag"
 import goprovider "github.com/pulumi/pulumi-go-provider"

-ctx.LogStatus(diag.Error, ...)
+goprovider.GetLogger(ctx).ErrorStatus(...)

@@
var goprovider identifier
var diag identifier
var ctx identifier
@@
 import diag "github.com/pulumi/pulumi/sdk/v3/go/common/diag"
 import goprovider "github.com/pulumi/pulumi-go-provider"

-ctx.LogStatusf(diag.Error, ...)
+goprovider.GetLogger(ctx).ErrorStatusf(...)

@@
var goprovider identifier
var diag identifier
var ctx identifier
@@
 import diag "github.com/pulumi/pulumi/sdk/v3/go/common/diag"
 import goprovider "github.com/pulumi/pulumi-go-provider"

-ctx.Log(diag.Debug, ...)
+goprovider.GetLogger(ctx).Debug(...)

@@
var goprovider identifier
var diag identifier
var ctx identifier
@@
 import diag "github.com/pulumi/pulumi/sdk/v3/go/common/diag"
 import goprovider "github.com/pulumi/pulumi-go-provider"

-ctx.Logf(diag.Debug, ...)
+goprovider.GetLogger(ctx).Debugf(...)

@@
var goprovider identifier
var diag identifier
var ctx identifier
@@
 import diag "github.com/pulumi/pulumi/sdk/v3/go/common/diag"
 import goprovider "github.com/pulumi/pulumi-go-provider"

-ctx.LogStatus(diag.Debug, ...)
+goprovider.GetLogger(ctx).DebugStatus(...)

@@
var goprovider identifier
var diag identifier
var ctx identifier
@@
 import diag "github.com/pulumi/pulumi/sdk/v3/go/common/diag"
 import goprovider "github.com/pulumi/pulumi-go-provider"

-ctx.LogStatusf(diag.Debug, ...)
+goprovider.GetLogger(ctx).DebugStatusf(...)

@@
var goprovider identifier
var diag identifier
var ctx identifier
@@
 import diag "github.com/pulumi/pulumi/sdk/v3/go/common/diag"
 import goprovider "github.com/pulumi/pulumi-go-provider"

-ctx.Log(diag.Warning, ...)
+goprovider.GetLogger(ctx).Warning(...)

@@
var goprovider identifier
var diag identifier
var ctx identifier
@@
 import diag "github.com/pulumi/pulumi/sdk/v3/go/common/diag"
 import goprovider "github.com/pulumi/pulumi-go-provider"

-ctx.Logf(diag.Warning, ...)
+goprovider.GetLogger(ctx).Warningf(...)

@@
var goprovider identifier
var diag identifier
var ctx identifier
@@
 import diag "github.com/pulumi/pulumi/sdk/v3/go/common/diag"
 import goprovider "github.com/pulumi/pulumi-go-provider"

-ctx.LogStatus(diag.Warning, ...)
+goprovider.GetLogger(ctx).WarningStatus(...)

@@
var goprovider identifier
var diag identifier
var ctx identifier
@@
 import diag "github.com/pulumi/pulumi/sdk/v3/go/common/diag"
 import goprovider "github.com/pulumi/pulumi-go-provider"

-ctx.LogStatusf(diag.Warning, ...)
+goprovider.GetLogger(ctx).WarningStatusf(...)

[blampe]

This is great! Let's make sure to update the boilerplate repo to reflect the new API.

Why not a resource controller as recommended in #159?

That sounds fine. I'll still probably embed some Logger structs for convenience, but that's something consumers can do to their liking.

ctx = integration.InjectTestLogger(ctx)

My only feedback is that it would be helpful to be able to inject a Logger interface, something like

ctx = integration.WithLogger(ctx, l)

and a default integration.TestLogger could be available.

This isn't part of this PR so there's no need to block.

I'm sorry to break existing users but I think this design is strictly better in the long term.

Strongly agree!

[iwahbe]

This is great! Let's make sure to update the boilerplate repo to reflect the new API.

Good call out!