Refactor internals for easier testing and less complexity

[pawelprazak]

Hello!

• Vote on this issue by adding a 👍 reaction
• If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.)

Issue details

This issue by nature probably will not have a complete description since we might learn new things while implementing and I might have forgotten some more things that should be added.

I'll do my best to keep this list up to date as we learn more.

The goals:

• allow for adding features and writing tests easily
• reduce internal complexity for less costly maintenance
• allow for maximum reuse and coherence between CLI, Automation and Tests

Non-goals:

• we do not want to diverge too much from TS and Go and other implementations
• we do not want any big changes to the public API

Problems and solutions are described below.

Focused mainly on Deployment:

• DeploymentImpl reads environment variables directly instead being configured using the constructor, this creates problems with testing and violates single responsibility principle - proposed solution is to read the environment variables in PulumiInternal and pass an "options object" (RuntimeContext?) with the result along with other dependencies to the constructor (this will also help with implementing the Automation API)
• DeploymentImpl creates its own dependencies internally and that does not allow for easy testing - proposed solution is to use manual dependency injection and pass all dependencies as parameters to the constructor, this allows for cleaner PulumiInternal and for a cleaner testing framework with less mocking
• DeploymentImpl holds its dependencies as nested classes, that makes the file big and hard to work with, encourages tight coupling between separate parts, does not allow for easy use in testing framework - proposed solution is to move all nested internal classes to package com.pulumi.runtime.internal and also extract interfaces for all of those classes (e.g Call, Invoke, etc.), as an additional benefit this will allow for easier understanding of the underlying interaction with the engine
• Deployment is being accessed through a singleton in many places, deep in the code structure, this makes testing harder requiring deployment instance in places where a unit test would be more appropriate - proposed solution is to reduce the amount of calls to the singleton by passing the instance from the edges of the code
• Deployment as a class name suggests that it is a thing, when in reality it is a state associated with an action, this is misleading - proposed solution is a rename to DeploymentContext and a move to package com.pulumi.context.internal
• Stack can probably be passed as a DeploymentImpl constructor parameter instead of through a setter, resulting in simplicity and immutability
• exposure of Deployment to the internals and providers that result in tight coupling could be reduced or eliminated by using "view" interfaces with only a slice of the api targeting a specific audience (e.g. only Call or only registering)

Focused on Resource and its subclasses:

• ComponentResource might benefit from making registerOutputs a template method to make it obvious to the user that they are required to implement it (instead of relying on the user to remember to call it)
• ResourceReference as a Resource super-class instead of a sub-class would remove accidental complexity
• Stack, StackReference and WorkspaceStack should have coherent APIs
• Stack should be final and not instantiatable directly by the user
• consider reuse of Stack class behind a WorkspaceStack interface for the Automation API

Other:

• Pulumi should get a companion interface with the public test framework API called PulumiTest that would contain mostly what DeploymentMockBuilder and DeploymentMock do right now; also backed by the same PulumiInternal, probably extended with PulumiTestInternal for any test specific implementations
• Pulumi should get a companion interface with the public Automation API called PulumiAuto or PulumiAutomation also backed by the same PulumiInternal

Affected area/feature

[t0yv0]

I'd request breaking user-facing changes out of this into separate tickets so we can start building up the CHANGELOG habits.

AFAIK:

• the users do not interact with Deployment, DeploymentImpl class, so all those changes are internal refactors

• Pulumi should get a companion interface with the public test framework API this needs a separate ticket, I think it's basically Add mock (unit) testing Java example #553 - we expect there another pass on mock testing API and an example checked into the repo, and sounds like there's been progress there, to make it a lot better

• Pulumi should get a companion interface with the public Automation API called PulumiAuto or PulumiAutomation also backed by the same PulumiInternal - needs a design doc on Automation API to cover this, I do not understand how this would work, especially since this is seems very different of how Automation API is done in every other Pulumi language

• ComponentResource might benefit from making registerOutputs a template method to make it obvious to the user that they are required to implement it (instead of relying on the user to remember to call it) - needs a separate ticket and needs some design alignment; that'd be a breaking change we need to be sure to want it; we'll need to update docs/examples also as part of rolling this out

• ResourceReference as a Resource super-class instead of a sub-class would remove accidental complexity needs separate ticket and an explanation.

• Stack, StackReference and WorkspaceStack should have coherent APIs - needs a separate ticket to discuss; what is WorkspaceStack? Also bear in mind Stack just became final/internal so consistency with Stack does not matter.

• Stack should be final and not instantiatable directly by the user - this is happening in Simplify Stack and Runner #642 it would seem; the change is good but it needs to be explained in the CHANGELOG

[pawelprazak]

I've added a targeted follow-up that is part of the topics described here: #668
It builds on top of #642 but needs more work to be sure we get the best API possible :)

Since it is user facing it deserves a separate issue IMO.