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

Ensure provider InvokeOption is passed to functions #1601

Merged
merged 2 commits into from
Jun 11, 2021

Conversation

jen20
Copy link
Contributor

@jen20 jen20 commented Jun 6, 2021

This commit ensure that provider options specified are passed to the YAML Decoding function if supplied to a ConfigGroup. This prevents the unnecessary instantiation of a default provider where this is used in a mode where only explicitly created providers are otherwise in play.

@github-actions
Copy link

github-actions bot commented Jun 6, 2021

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.

Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

This commit ensure that provider options specified are passed to the
YAML Decoding function if supplied to a ConfigGroup. This prevents the
unnecessary instantiation of a default provider where this is used in a
mode where only explicitly created providers are otherwise in play.
@jen20 jen20 force-pushed the jen20/fix-invokeopts-yaml-decode branch from ebecc4b to 4d72a1d Compare June 6, 2021 00:02
@github-actions
Copy link

github-actions bot commented Jun 6, 2021

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.

Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@infin8x infin8x requested a review from lblackstone June 8, 2021 23:09
@infin8x
Copy link

infin8x commented Jun 8, 2021

@lblackstone can you take a look at this please?

@lblackstone
Copy link
Member

/run-acceptance-tests

@github-actions
Copy link

github-actions bot commented Jun 9, 2021

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.

Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@github-actions
Copy link

github-actions bot commented Jun 9, 2021

@lblackstone
Copy link
Member

Changes LGTM, but CI testing is currently blocked on #1607

@lblackstone lblackstone merged commit 54c8f7a into pulumi:master Jun 11, 2021
@lblackstone
Copy link
Member

(Tested the changes in #1613)

@jen20 jen20 deleted the jen20/fix-invokeopts-yaml-decode branch June 12, 2021 04:22
EronWright added a commit that referenced this pull request Jan 3, 2024
<!--Thanks for your contribution. See [CONTRIBUTING](CONTRIBUTING.md)
    for Pulumi's contribution guidelines.

    Help us merge your changes more quickly by adding more details such
    as labels, milestones, and reviewers.-->

### Proposed changes
Epic: #2254
Fixes: #2710

This PR standardizes the option propagation logic for the component
resources in the pulumi-kubernetes Go SDK. The general approach is:
1. In the component resource constructor, compute the child options to
be propagated to any children. The child options consist of the
component as parent, and with `version` and `pluginDownloadURL` if
specified.
2. Compute the invoke options by copying the child options.

### Specification
The component resource is responsible for computing sub-options for
invokes and for child resource declarations. This table outlines the
expected behavior for each [resource
option](https://www.pulumi.com/docs/concepts/options/) when presented to
a component resource.

|  | Propagated | Remarks |
|---|---|---|
| `additionalSecretOutputs` | no | "does not apply to component
resources" |
| `aliases` | no | Inherited via parent-child relationship. |
| `customTimeouts` | no | "does not apply to component resources" |
| `deleteBeforeReplace` | no |  |
| `deletedWith` | no |  |
| `dependsOn` | no | The children implicitly wait for the dependency. |
| `ignoreChanges` | no | Nonsensical to apply directly to children (see
[discussion](pulumi/pulumi#8969)). |
| `import` | no |  |
| `parent` | **yes** | The component becomes the parent. |
| `protect` | no | Inherited (see [p/p
bug](pulumi/pulumi#12431)). |
| `provider` | no | Combined into providers map, then inherited via
parent-child relationship. |
| `providers` | no | Inherited. |
| `replaceOnChanges` | no | "does not apply to component resources" |
| `retainOnDelete` | no | "does not apply to component resources" |
| `transformations` | no | Inherited. |
| `version` | **yes** | Influences default provider selection logic
during invokes.<br/>Should propagate when child resource is from the
same provider type. |
| `pluginDownloadURL` | **yes** | Influences default provider selection
logic during invokes.<br/>Should propagate when child resource is from
the same provider type. |

### Testing
A new test case is provided ([test
case](https://github.com/pulumi/pulumi-kubernetes/blob/073b9dc64e32e4f14fa6691e1b11049007ca2db7/tests/sdk/go/go_test.go#L808),
[test
program](https://github.com/pulumi/pulumi-kubernetes/blob/073b9dc64e32e4f14fa6691e1b11049007ca2db7/tests/sdk/go/options/main.go))
that exercises option propagation across the component resources:
-
[kubernetes.helm.sh.v3.Chart](https://www.pulumi.com/registry/packages/kubernetes/api-docs/helm/v3/chart/)
-
[kubernetes.kustomize.Directory](https://www.pulumi.com/registry/packages/kubernetes/api-docs/kustomize/directory/)
-
[kubernetes.yaml.ConfigGroup](https://www.pulumi.com/registry/packages/kubernetes/api-docs/yaml/configgroup/)
-
[kubernetes.yaml.ConfigFile](https://www.pulumi.com/registry/packages/kubernetes/api-docs/yaml/configfile/)

Upgrade testing must be done manually, with an emphasis on avoiding
replacement due to reparenting.

### Related issues (optional)

The Go SDK doesn't allow for options to be mutated via the
"kubernetes-style" transformation function.
#2666

During testing it was observed that the `parent` field is occasionally
missing from `RegisterResource` RPC call.
pulumi/pulumi#14826

Here's some key related PRs for additional context:
- pulumi/pulumi#8796
- #1601
- #1919
- #2005
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