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

Improve provider option propagation for component resources #2624

Closed
wants to merge 31 commits into from

Conversation

EronWright
Copy link
Contributor

@EronWright EronWright commented Oct 24, 2023

Proposed changes

Closes #2254
Closes #2345

Fixes a few related aspects of options on component resources:

  • allow the use of provider or providers on the options
  • don't propagate all the options on the component to the child; be selective and consistent
  • be consistent about the invoke options

To better encapsulate the logic, each SDK now uses a pair of utility functions (e.g. GetChildOptions, GetInvokeOptions), to be used by the various resource types. Hopefully it is more clear that, at runtime, ComponentResourceOptions becomes CustomResourceOptions (for the nascent children) which then becomes InvokeOptions (with the component as parent).

Impacts of this PR:

  1. In some edge cases, some child resources will have different options, because a parent's option was (erroneously) being propagated. e.g. protect.
  2. The child resources now uniformly depend on the dependencies of the component resource. Previously this was true of Kustomize only.

TODO:

  • Go SDK
  • NodeJS SDK
  • Python SDK
  • DotNet SDK
  • Tests - Go SDK
  • Tests - NodeJS SDK
  • Tests - Python SDK
  • Tests - DotNet SDK
  • Undo of "nested ConfigFile" change

Related issues (optional)

Related to #1833 in that this PR will set the dependOn option on the chart resources, which is one variation of the original problem.

Loosely related to #2666 and pulumi/pulumi#8969.

Fixes

Here's a list of issues that are believed to be fixed by this PR.

Python SDK

Error: Default provider for 'kubernetes' disabled. 'kubernetes:yaml:decode' must use an explicit provider.

In some circumstances, the SDK attempts to use the default provider despite being configured otherwise. If the default provider is disabled, deployment fails with:

    error: Program failed with an unhandled exception:
    Traceback (most recent call last):
      File "./__main__.py", line 50, in <module>
        k8s.yaml.ConfigGroup(
      File "./venv/lib/python3.11/site-packages/pulumi_kubernetes/yaml/yaml.py", line 185, in __init__
        cf = ConfigFile(
             ^^^^^^^^^^^
      File "./venv/lib/python3.11/site-packages/pulumi_kubernetes/yaml/yaml.py", line 348, in __init__
        __ret__ = invoke_yaml_decode(text, invoke_opts)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "./venv/lib/python3.11/site-packages/pulumi_kubernetes/yaml/yaml.py", line 1951, in invoke_yaml_decode
        inv = pulumi.runtime.invoke('kubernetes:yaml:decode', {'text': text}, invoke_opts)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "./venv/lib/python3.11/site-packages/pulumi/runtime/invoke.py", line 190, in invoke
        raise invoke_error
    Exception: invoke of kubernetes:yaml:decode failed: Invoke: Default provider for 'kubernetes' disabled. 'kubernetes:yaml:decode' must use an explicit provider.

NodeJS SDK

Error: Default provider for 'kubernetes' disabled. 'kubernetes:yaml:decode' must use an explicit provider.

In some circumstances, the SDK attempts to use the default provider despite being configured otherwise. If the default provider is disabled, deployment fails with:

    error: Error: Invoke: Default provider for 'kubernetes' disabled. 'kubernetes:yaml:decode' must use an explicit provider.

Support for providers option on ConfigGroup:

Previously, the providers option on ConfigGroup was wrongly propagated to the inner ConfigMap, yielding an error. Given:

new k8s.yaml.ConfigGroup(
    "cg-a",
    {
        yaml: [`
apiVersion: v1
kind: ConfigMap
metadata:
  name: cg-a-cm-1
`],
    },
    {
        providers: [aProvider],
    });

Old behavior:

    error: Error: Do not supply 'providers' option to a CustomResource. Did you mean 'provider' instead?
        at new CustomResource (./node_modules/@pulumi/resource.ts:891:19)
        at new ConfigMap (./node_modules/@pulumi/core/v1/configMap.ts:91:9)
        at ./node_modules/@pulumi/yaml/yaml.ts:3625:27

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@codecov-commenter
Copy link

codecov-commenter commented Oct 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a65574d) 18.73% compared to head (421488d) 18.62%.
Report is 41 commits behind head on master.

❗ Current head 421488d differs from pull request most recent head c15bd22. Consider uploading reports for the commit c15bd22 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2624      +/-   ##
==========================================
- Coverage   18.73%   18.62%   -0.11%     
==========================================
  Files          47       47              
  Lines        9535     9589      +54     
==========================================
  Hits         1786     1786              
- Misses       7645     7699      +54     
  Partials      104      104              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pulumi-bot pulumi-bot added the needs-triage Needs attention from the triage team label Oct 24, 2023
Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

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

Looks good so far!

@EronWright EronWright force-pushed the eronwright/issue-2254 branch from 1513075 to 421488d Compare November 30, 2023 06:23
@EronWright
Copy link
Contributor Author

Refactoring this PR into a PR per SDK.

@EronWright EronWright closed this Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage Needs attention from the triage team
Projects
None yet
4 participants