chore(sources): Add SourceContext

[jszwedko]

To inject the Resolver similar to the TransformContext. I need the
Resolver in the new aws_s3 source. I figured I'd submit this change
separately since it was easy to pull out and I wanted thoughts sooner
than later.

Alternatively, it seems like the resolver is static now so we could drop
it from SinkContext and TransformContext. I wanted some confirmation
on that front though. I'm happy to reverse this and remove injection of
the Resolver.

Signed-off-by: Jesse Szwedko jesse@szwedko.me

[lukesteensen]

Since Resolver is already halfway removed, I don't think it's worth adding SourceContext just for that. We may well need it for other things later, but better to defer until that time.

In the source where you need it, I would just create one yourself.

[jszwedko]

@lukesteensen makes sense, thanks for the validation! Would it be worth me creating a PR to drop the Resolver in the rest of the places?

[ktff]

We decided in #2812 to keep passing Resolver for flexibility of future changes to it, so I would leave that in for now, but I completely agree with what @lukesteensen said.

[jszwedko]

Hmm, actually, given @ktff's reference to the decision in #2812 I'm actually inclined to add the SourceContext here with the Resolver given that we wanted to keep it in the TransformContext and SinkContext for flexibility. I'd prefer going either one way or the other rather (injecting it everywhere or injecting it nowhere) than having it only injected sometimes as it is liable to be confusing when working in the codebase.

[lukesteensen]

I did say that, didn't I...

To be clear, I think the intention of that comment was that we continue to provide a resolver interface, not that we necessarily continue to thread it through contexts. The mentioned benefit of threading an executor is not really a valid one anymore, and we have definitely decided not to let the behavior be configurable. So it would be perfectly fine in my opinion to have something as simple as a top-level async fn resolve that can be used wherever needed.

On the other hand, it doesn't really matter that much. We should be consistent and there's no urgency to moving away from the threaded resolvers on other contexts, so I would be totally fine going in this direction for now and revisiting later.

[jszwedko]

To be clear, I think the intention of that comment was that we continue to provide a resolver interface, not that we necessarily continue to thread it through contexts.

👍 Sorry, that was my mistake in not being clearer. I meant to ask if I should, instead of adding the SourceContext, drop injecting the resolver in the TransformContext and SinkContexts to be consistent.

It's definitely not urgent, but the rough edge there did lead to some confusion on my part, initially, and I can see it doing the same for others.

This is the comment that led me to believe that adding SourceContext would be preferable: #2812 (review)

I'm fine to go either way though 😄

[lukesteensen]

I meant to ask if I should, instead of adding the SourceContext, drop injecting the resolver in the TransformContext and SinkContexts to be consistent.

Yes, I meant to communicate that this is my preference, sorry 😄

[jszwedko]

😄

I'll close this and open a new PR for removing it.