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

feat: enable imports from nodejs provider resources #14668

Merged
merged 3 commits into from
Nov 30, 2023

Conversation

bfraterman-tkhsecurity
Copy link
Contributor

@bfraterman-tkhsecurity bfraterman-tkhsecurity commented Nov 27, 2023

Description

I was writing a typescript resource provider, but got errors that the the plugin didn't support imports, while the read method was implemented on the provider.
This PR also sets the Inputs on the ReadResponse, so imports will be supported.

Fixes # (issue)
Could not find existing issue

Checklist

  • I have run make tidy to update any new dependencies

  • I have run make lint to verify my code passes the lint check

  • I have formatted my code using gofumpt
    (None of the above, since the change is in js code)

  • I have added tests that prove my fix is effective or that my feature works
    There are no existing tests for the server.ts file. If really required for merging I could maybe take a look at a test for this use-case.

  • I have run make changelog and committed the changelog/pending/<file> documenting my change

  • Yes, there are changes in this PR that warrants bumping the Pulumi Cloud API version

Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation

@pulumi-bot
Copy link
Contributor

pulumi-bot commented Nov 27, 2023

Changelog

[uncommitted] (2023-11-29)

Features

  • [sdk/nodejs] Enable resource imports for nodejs providers

@Frassle
Copy link
Member

Frassle commented Nov 27, 2023

but got errors that the the plugin didn't support imports,

What errors exactly? "inputs" are not "props" and are in general not required for the engine to do an import. Which also makes this change a bit suspicious, because the props returned by "read" might not be the right thing to also tell the engine are inputs.

@bfraterman-tkhsecurity
Copy link
Contributor Author

I got the error originating from here: https://github.com/pulumi/pulumi/blob/master/pkg/resource/deploy/step.go#L1037

I'm very unfamiliar with the pulumi codebase. Do you think it's maybe better to change the ReadResult interface in the node SDK and expand it with an inputs property?

Do we have an example (from maybe another SDK) what that should look like?

@Frassle
Copy link
Member

Frassle commented Nov 29, 2023

Do you think it's maybe better to change the ReadResult interface in the node SDK and expand it with an inputs property?

Yeh that's probably more correct. It's what the C# SDK does (https://github.com/pulumi/pulumi-dotnet/blob/main/sdk/Pulumi/Provider/Provider.cs#L217-L222) and the Go SDK (https://github.com/pulumi/pulumi-go-provider/blob/main/infer/resource.go#L113-L114)

Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation

@bfraterman-tkhsecurity
Copy link
Contributor Author

Yeh that's probably more correct. It's what the C# SDK does (https://github.com/pulumi/pulumi-dotnet/blob/main/sdk/Pulumi/Provider/Provider.cs#L217-L222) and the Go SDK (https://github.com/pulumi/pulumi-go-provider/blob/main/infer/resource.go#L113-L114)

Fixed that, please let me know if you have any more concerns.

@Frassle
Copy link
Member

Frassle commented Nov 29, 2023

Not sure if you would call this feature 'user-facing' since it's about plugin development.

Provider developers are users too :) Add a changelog for this and we can get it merged.

Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation

@Frassle Frassle added this pull request to the merge queue Nov 29, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 29, 2023
@Frassle Frassle added this pull request to the merge queue Nov 29, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 29, 2023
@Frassle Frassle added this pull request to the merge queue Nov 29, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 30, 2023
@bfraterman-tkhsecurity
Copy link
Contributor Author

@Frassle Any clue what's going on with the build pipeline? Doesn't seem likely that it's caused by these changes?

@Frassle
Copy link
Member

Frassle commented Nov 30, 2023

CI pipeline is known to be flakey at the moment, I can just keep retrying this till it goes through.

@Frassle Frassle added this pull request to the merge queue Nov 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 30, 2023
@Frassle Frassle added this pull request to the merge queue Nov 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 30, 2023
@Frassle Frassle added this pull request to the merge queue Nov 30, 2023
Merged via the queue into pulumi:master with commit 25f1e52 Nov 30, 2023
8 of 9 checks passed
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.

None yet

3 participants