Add context.Context to deployment.Environment.

[cgruber]

We expose the context.Context via a lambda/pure-function on deployment.Environment. Mostly the ctx was already present where we create development.Environment (for other reasons), but now we expose it through the environment for attaching to remote-calls that aren't already managed by a wrapper client. Or, for whatever other things changesets may need to attach to the context for. (We are considering global timeouts for changset operations, for instance)

Plumb this through in the test environment and devenv as well in chainlink/deployments as well.

TODO: Apply a more robust cancellation signal handling, rather than dumbly plumbing through existing outer contexts (e.g. a cancellation method on Environment, and registering it where important). This will be done in a subsequent PR.

[github-actions]

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

[jmank88]

We need to be careful with how context is wired through. We should be able to follow the package docs without exception:

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx

A common problem that can make this seem difficult is having constructors that also do operations depending on the context. If the plain construction is kept separate from the operations that use the context, then it will be easy to wire it everywhere trivially (and easier to test, compose, etc).

[jmank88]

We should be able to follow the package docs without exception:

I said "should" but in a formal sense this is literally always possible, e.g. by passing a closure func(fn func(ctx.Context)) to invoke things with the context. This may seem like a hack outside of exceptions like channel communication, but wiring it up that way first often illuminates a better approach.

[jmank88]

When these constructors are called normally, is there always an appropriate context on hand to pass in already? If not (or even if so?), this signature could be simplified by making a new context internally and returning (or providing a method to invoke) cancel().

Suggested change

	env := memory.NewMemoryEnvironmentFromChainsNodes(func() context.Context { return ctx }, lggr, allChains, allNodes)
	env, cancel := memory.NewMemoryEnvironmentFromChainsNodes(lggr, allChains, allNodes)
	t.Cleanup(cancel)

This is also compatible with cases where there is a context already:

Suggested change

	env := memory.NewMemoryEnvironmentFromChainsNodes(func() context.Context { return ctx }, lggr, allChains, allNodes)
	env := memory.NewMemoryEnvironmentFromChainsNodes(lggr, allChains, allNodes)
	context.AfterFunc(ctx, env.Cancel)

[cgruber]

There is already a context in the other case - I plan to normalize them. I am trying to keep things somewhat stable. I have a lot of change to stack on this, and I'm trying to not change too much of the signatures in one PR. I want to do a pass through all of this. They were pulling a context off of testing.T internally, and this was intended to make them both work the same way, pending future fixes. I'm not trying to fix all the things in this PR.

This is a good pattern to keep in mind, however, and I'll look at it as an option for a followup.

[cgruber]

Also, that doesn't exactly simplify the signature, it just moves a parameter to a return type, so it's the same number of parameters (if you count return values as "out" params)

[cgruber]

It does simplify having to construct a lambda, but I have a cleanup there planned as well.

[jmank88]

Please consider the method variant.

[cgruber]

Per our discussion, I'll definitely look at the method variant, but for now, plumbing through the lambda is enough to unblock other work, and I'll make a point of revisiting this for a more mature signal handling once we're out of the crunch.

[jmank88]

This is fine as-is, but declaring one ctx := ctx() at the top of the func can be less error prone over time.

[jmank88]

Ah, but this is a more complicated case where the context is being used in the constructor rather than a method. This is often problematic because it leaves the user unable to control the two separately. I think it would be appropriate to pass a context normally to these calls that use it synchronously. But OFC that would be messy alongside the func() context.Context, so probably only along with https://github.com/smartcontractkit/chainlink/pull/15410/files#r1859485133

[cgruber]

I agree in principle - my plan was to fix those constructors in a subsequent PR, where possible (or use the mentioned pattern). This was an enabling change for some other situations where contexts are being used, but naively in the middle of downstream code in this and chainlink-deployments, and I wanted the plumbing all the way through, to at least make sure we HAD cancellation, and had a way to avoid folks using weird lambdas (not like what you're suggesting, but as complete workarounds of the outer API, to plumb through things like the context, and certain secrets, etc.).

Now that I understand context better, I can do a cleanup of context more generally of code in /development and chainlink-deployments.