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

fix(pulumi): return correct responses for pulumi plugin commands #5129

Merged
merged 2 commits into from Oct 2, 2023

Conversation

shumailxyz
Copy link
Contributor

@shumailxyz shumailxyz commented Sep 22, 2023

What this PR does / why we need it:
This enforces responses of type ValidResultType for the pulumi plugin commands for actions. Previously, the plugin commands for actions were not returning anything, resulting in runtime errors. The TS compiler didn't catch these due to the pulumi plugin command responses typed as any.
Which issue(s) this PR fixes:

Fixes #5118
Fixes #5084

Special notes for your reviewer:

@shumailxyz shumailxyz requested a review from a team September 22, 2023 16:20
core/src/graph/nodes.ts Outdated Show resolved Hide resolved
@shumailxyz shumailxyz marked this pull request as draft September 25, 2023 13:54
this enforces responses of type ValidResultType for the pulumi plugin commands for actions.
@shumailxyz shumailxyz changed the title fix(graph): safely check state of a node fix(pulumi): return correct responses for pulumi plugin commands Oct 2, 2023
@shumailxyz shumailxyz marked this pull request as ready for review October 2, 2023 10:40
Copy link
Member

@stefreak stefreak left a comment

Choose a reason for hiding this comment

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

Awesome work @shumailxyz!

I'd love to see tests for this, but as the pulumi provider is still flagged as experimental I am ok with this as-is; it's already a big improvement and will help catch this class of bugs during compilation.

@shumailxyz shumailxyz added this pull request to the merge queue Oct 2, 2023
@shumailxyz
Copy link
Contributor Author

Totally agreed. We have a separate issue #4600 to track tests for pulumi, should consider prioritizing that.

Merged via the queue into main with commit da981de Oct 2, 2023
41 checks passed
@shumailxyz shumailxyz deleted the pulumi-undefined-err branch October 2, 2023 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants