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

[sdk] Implement reflection-based RegisterOutputs for component resources #200

Merged
merged 3 commits into from
Nov 1, 2023

Conversation

Zaid-Ajaj
Copy link
Contributor

@Zaid-Ajaj Zaid-Ajaj commented Oct 31, 2023

When writing component resources, at the very end of the constructor logic, developers have to make a call to RegisterOutputs(...) and provide a bag of outputs from the instance properties declared with [Output] property.

This PR makes it such that developers can use RegisterOutputs() without providing anything and it will use reflection to deduce the keys and values of the outputs from the properties of the component resource. It works as follows:

  • Any property of type Output<T> is considered an output with the key equal to the name of the property
  • Any property marked with attribute [Output] is considered an output the key equal to the name of the property
  • Any property marked with attribute [Output(name: "<overriddenPropertyName>")] is considered an output the key equal to <overriddenPropertyName>

Since we don't have a good way to test RegisterOutputs, I moved the logic to an internal, static function called CollectOutputProperties which is unit testable on its own. Then in a test stack, made it return the stack outputs from the collected output properties of a test component (BasicComponent) so we can assert the resulting keys and values

The PR extends IMocks with support for RegisterResourceOutputs such that we can unit test the registered outputs by component resources or stacks

@Zaid-Ajaj Zaid-Ajaj added the kind/enhancement Improvements or new features label Oct 31, 2023
@Zaid-Ajaj Zaid-Ajaj requested a review from a team October 31, 2023 15:36
@Zaid-Ajaj Zaid-Ajaj force-pushed the zaid/reflection-based-register-outputs branch from 0070dfb to 6f78792 Compare October 31, 2023 15:38
@Frassle
Copy link
Member

Frassle commented Oct 31, 2023

Since we don't have a good way to test RegisterOutputs

Really? Can't you test this in a mock stack?

sdk/Pulumi/Resources/ComponentResource.cs Outdated Show resolved Hide resolved
sdk/Pulumi/Resources/ComponentResource.cs Outdated Show resolved Hide resolved
@Frassle Frassle added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Oct 31, 2023
@Frassle
Copy link
Member

Frassle commented Oct 31, 2023

no-changlog-required because changelog bot doesn't understand changie yet.

sdk/Pulumi/Testing/IMocks.cs Outdated Show resolved Hide resolved
sdk/Pulumi/Testing/IMocks.cs Outdated Show resolved Hide resolved
sdk/Pulumi/Resources/ComponentResource.cs Show resolved Hide resolved
sdk/Pulumi/Resources/ComponentResource.cs Outdated Show resolved Hide resolved
sdk/Pulumi.Tests/Resources/ComponentResourceTests.cs Outdated Show resolved Hide resolved
sdk/Pulumi.Tests/Resources/ComponentResourceTests.cs Outdated Show resolved Hide resolved
@Zaid-Ajaj Zaid-Ajaj added this pull request to the merge queue Nov 1, 2023
Merged via the queue into main with commit 433529d Nov 1, 2023
8 checks passed
@Zaid-Ajaj Zaid-Ajaj deleted the zaid/reflection-based-register-outputs branch November 1, 2023 13:48
jkerken pushed a commit to algompluecker/pulumi-dotnet that referenced this pull request Jul 24, 2024
…ces (pulumi#200)

* Implement reflection-based RegisterOutputs() for component resources

* Extend IMocks with RegisterResourceOutputs to allow unit testing registered outputs

* Outputs in MockRegisterResourceOutputsRequest are now typed as Output<object?>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update kind/enhancement Improvements or new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants