Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Improve all public APIs by dropping (using Context) #538

Merged
merged 40 commits into from
Oct 2, 2024

Conversation

lbialy
Copy link
Collaborator

@lbialy lbialy commented Sep 6, 2024

The (using Context) clause and related context function executed in Pulumi.run are both a blessing as they make the task of propagation of the main data structure quite easy and a curse as they pollute public API with a detail that can't be used in any way by the user. @jchapuis suggested to move the main monad (from user's perspective) - the Output - towards a Kleisli-like structure and I was wary of that as it looked like it would break one of the core invariants of all async tasks being tracked by the Context to prevent early exits. There wasn't also much motivation to do such change beyond the simplification of the API but then we've got some reports about how Intellij IDEA's Scala Plugin fails to handle most of our APIs.

This PR changes Output from a wrapper over Result[OutputData[A]] to a wrapper over Context => Result[OutputData[A]]. This in turn allows us to drop (using Context) on Output constructors and demand an instance of Context only when interpreting Output to get the underlying OutputData[A] or just Option[A]. This interpretation happens in the guts of besom-core library and therefore this change should be invisible for the user (beyond the disappearance of (using Context) clause).

06.09.2024 / 70d2538 Current state is that Output was modified and all the changes to make the tests green have been applied. Task tracking is applied by registering the top-level tasks that are getting interpreted inside of the get* family of functions. The logic behind this is that since the final task waits for all tasks it composes it should be fine to do only this but this probably won't work for fire-and-forget cases where subtasks are launched in composed effects as it's possible we won't track their final Outputs at all. This is not a problem, we can modify the implementation so that all tasks are tracked the same way it was done before by just transforming the received function to track the resulting Result[OutputData[A]]. This comes at an increased memory cost but that's probably not a big deal given the amount of real parallelism that happens in Besom programs.

@lbialy lbialy requested a review from prolativ September 6, 2024 16:07
@lbialy lbialy changed the title Improve all public APIs by dropping (using Context) WIP: Improve all public APIs by dropping (using Context) Sep 8, 2024
@lbialy lbialy removed the request for review from prolativ September 8, 2024 22:14

def ofData[A](data: OutputData[A]): Output[A] = new Output(ctx => Result.pure(data))

def secret[A](value: A): Output[A] = new Output(ctx => Result.pure(OutputData(value, Set.empty, isSecret = true)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For completeness, should we have something like def deferSecret?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, internal API, if we don't use something (we won't use this method), it should not exist.

def trace(message: LoggableMessage, res: Option[Resource] = None, streamId: Int = 0, ephemeral: Boolean = false)(using
pkg: sourcecode.Pkg,
fileName: sourcecode.FileName,
name: sourcecode.Name,
line: sourcecode.Line,
mdc: BesomMDC[_] = BesomMDC.empty
mdc: BesomMDC[_] = BesomMDC.empty,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it won't be possible to call this method without mdc, unless one passes ctx as a named argument. This is a source incompatible change. Was this supposed to work like that now? ctx isn't used actually at the moment

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a public API. This logger is purely internal, it's not even used by published packages.

@@ -10,7 +10,7 @@ object interpolator:

implicit final class PulumiInterpolationOps(sc: StringContext):
def pulumi(args: Any*)(using Context): Output[String] =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't (using Context) be unnecessary now (here and for p)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's WIP, I haven't completed the search for all (using Context) clauses in public APIs yet.

@lbialy lbialy merged commit 96f1f75 into main Oct 2, 2024
4 checks passed
@lbialy lbialy deleted the no-implicit-context branch October 2, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants