-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Added support for custom naming of dynamic provider resource #7633
Added support for custom naming of dynamic provider resource #7633
Conversation
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
1 similar comment
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
Thanks for the PR! I believe this would address #3468. We likely want to add the same support for Node.js as well as part of any update here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some tests for this as well? An integration test like tests/integration/dynamic/python
should work. To add a test, you can just copy that folder to a new folder (e.g. tests/integration/dynamic/python-type-name
) and add a new test method to tests/integration/integration_python_test.go
, e.g. by copying and modifying the method for tests/integration/dynamic/python
.
Please let me know if you need any more info on how to add a test!
@lukehoban I think that we can file a follow-up issue for Node.
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
4f0fcc8
to
41cb09c
Compare
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit, but LGTM otherwise!
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
1 similar comment
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
/run-acceptance-tests |
Please view the results of the PR Build + Acceptance Tests Run Here |
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
56237a6
to
e9efab1
Compare
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
e9efab1
to
92656e4
Compare
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
92656e4
to
1ad23e5
Compare
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
1ad23e5
to
a41f145
Compare
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
@pgavlin I've added a test. It is my first contribution in go and I don't know how to run tests. I've tried to run |
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small set of suggestions for the test changes, but this looks great!
You can ensure that your test runs by running go test -tags=all -run TestCustomResourceTypeNameDynamicPython
from within the tests/integration
directory.
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
Co-authored-by: Pat Gavlin <pgavlin@gmail.com>
Co-authored-by: Pat Gavlin <pgavlin@gmail.com>
Co-authored-by: Pat Gavlin <pgavlin@gmail.com>
249086a
to
4b34aae
Compare
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
4b34aae
to
4897dba
Compare
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
/run-acceptance-tests |
Please view the results of the PR Build + Acceptance Tests Run Here |
PR is now waiting for a maintainer to run the acceptance tests. Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR Further commands available:
|
/run-acceptance-tests |
Please view the results of the PR Build + Acceptance Tests Run Here |
@pgavlin @lukehoban it looks like this did indeed fix #3468 - shall I resolve it or is there more work tracked there? |
11498: area/cli: removed checks for PULUMI_DISABLE_RESOURCE_REFERENCES in cli r=RobbieMcKinstry a=stefins Removed checks for `PULUMI_DISABLE_RESOURCE_REFERENCES` env in cli and engine Related to #11385 <!--- Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation. --> # Description <!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. --> partially fixes #11385 (issue) ## Checklist <!--- Please provide details if the checkbox below is to be left unchecked. --> - [ ] I have added tests that prove my fix is effective or that my feature works <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Service, then the service should honor older versions of the CLI where this change would not exist. You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add it to the service. --> - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> 11873: [sdk/nodejs] - Add support for custom naming of dynamic provider resource r=RobbieMcKinstry a=Asamsig <!--- Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation. --> # Description <!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. --> Fixes #11856 Added two new optional constructor parameters to `pulumi.dynamic.Resource`, `module`, and `type`, which enables custom type naming of dynamic provider resources. Based on the Python implementation from [here](#7633). ### Before All dynamic provider resources would have the type `pulumi-nodejs:dynamic:Resource` ### After Usage: ```ts class CustomResource extends pulumi.dynamic.Resource { constructor (name: string, opts?: pulumi.ResourceOptions) { super(new DummyResourceProvider(), name, {}, opts, "custom-provider", "CustomResource") } } ``` Output: `pulumi-nodejs:dynamic/custom-provider:CustomResource` ## Checklist <!--- Please provide details if the checkbox below is to be left unchecked. --> - [x] I have added tests that prove my fix is effective or that my feature works <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Service, then the service should honor older versions of the CLI where this change would not exist. You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add it to the service. --> - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> Co-authored-by: STEFIN <stefin@pm.me> Co-authored-by: Alexander Samsig <koresamsig@gmail.com>
11873: [sdk/nodejs] - Add support for custom naming of dynamic provider resource r=RobbieMcKinstry a=Asamsig <!--- Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation. --> # Description <!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. --> Fixes #11856 Added two new optional constructor parameters to `pulumi.dynamic.Resource`, `module`, and `type`, which enables custom type naming of dynamic provider resources. Based on the Python implementation from [here](#7633). ### Before All dynamic provider resources would have the type `pulumi-nodejs:dynamic:Resource` ### After Usage: ```ts class CustomResource extends pulumi.dynamic.Resource { constructor (name: string, opts?: pulumi.ResourceOptions) { super(new DummyResourceProvider(), name, {}, opts, "custom-provider", "CustomResource") } } ``` Output: `pulumi-nodejs:dynamic/custom-provider:CustomResource` ## Checklist <!--- Please provide details if the checkbox below is to be left unchecked. --> - [x] I have added tests that prove my fix is effective or that my feature works <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Service, then the service should honor older versions of the CLI where this change would not exist. You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add it to the service. --> - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> Co-authored-by: Alexander Samsig <koresamsig@gmail.com>
Description
Now there is not possible to change a name of dynamic provider resource without copying a code of the
pulumi.sdk.python.lib.pulumi.dynamic.dynamic.Resource
and changing the hard-coded name"pulumi-python:dynamic:Resource"
. I successfully uses this proposal to make it possible.Usage:
Checklist