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

Prune read-only fields from resource inputs #2571

Merged
merged 6 commits into from
Sep 21, 2023
Merged

Conversation

EronWright
Copy link
Contributor

Proposed changes

This PR improves the handling of the read-only metadata fields of Kubernetes objects, such as creationTimestamp and resourceVersion. The provider now ignores these fields when they appear as resource inputs, to address a few quirks:

  1. Unparseable values do not cause an API Server error during preview.
  2. A subsequent refresh does not show a difference to the __inputs property.
  3. The behavior is now uniform for standard resource types and for custom resource types.

An integration test is provided to show that such fields are ignored as inputs, and are still available as outputs. Manual tests confirmed identical behavior for custom resource objects.

The resultant state shows that the read-only fields are not present in "inputs" nor in "__inputs", and are present in "outputs":

{
                "urn": "urn:pulumi:dev::issue-2351-simple-ts::kubernetes:apps/v1:Deployment::nginx",
                "custom": true,
                "id": "default/nginx-78a03128",
                "type": "kubernetes:apps/v1:Deployment",
                "inputs": {
                    "apiVersion": "apps/v1",
                    "kind": "Deployment",
                    "metadata": {
                        "annotations": {
                            "pulumi.com/autonamed": "true"
                        },
                        "name": "nginx-78a03128",
                        "namespace": "default"
                    },
                },
                "outputs": {
                    "__fieldManager": "pulumi-kubernetes-42ad785a",
                    "__initialApiVersion": "apps/v1",
                    "__inputs": {
                        "apiVersion": "apps/v1",
                        "kind": "Deployment",
                        "metadata": {
                            "annotations": {
                                "pulumi.com/autonamed": "true"
                            },
                            "name": "nginx-78a03128",
                            "namespace": "default"
                        },
                    },
                    "apiVersion": "apps/v1",
                    "kind": "Deployment",
                    "metadata": {
                        "annotations": {
                            "deployment.kubernetes.io/revision": "1",
                            "pulumi.com/autonamed": "true"
                        },
                        "creationTimestamp": "2023-09-19T23:58:41Z",
                        "generation": 1,
                        "name": "nginx-78a03128",
                        "namespace": "default",
                        "resourceVersion": "559181",
                        "uid": "ec381c4d-92a7-470a-8fdc-824cd22a59dc"
                    },
                },
            }

Related issues (optional)

Closes #2351

Related:

@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 Sep 20, 2023

Codecov Report

Merging #2571 (69cb28d) into master (d2a3b8c) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2571      +/-   ##
==========================================
- Coverage   18.64%   18.61%   -0.04%     
==========================================
  Files          47       47              
  Lines        9434     9440       +6     
==========================================
- Hits         1759     1757       -2     
- Misses       7576     7584       +8     
  Partials       99       99              
Files Changed Coverage Δ
provider/pkg/clients/unstructured.go 59.04% <ø> (-0.77%) ⬇️
provider/pkg/provider/provider.go 7.37% <0.00%> (-0.03%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor Author

@EronWright EronWright Sep 20, 2023

Choose a reason for hiding this comment

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

I moved the pruning logic to normalizeInputs in provider.go for two reasons:

  1. Pruning of read-only fields makes sense only for inputs, not for outputs, but the clients.Normalize function seems general-purpose in nature. That is, general-purpose normalization is independent of input pruning.
  2. It is desirable to prune the read-only fields even for custom resources, where a scheme isn't available (and shouldNormalize would be false).

I renamed normalize to normalizeInputs in provider.go for clarity, and double-checked that it is not used with outputs.

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.

Changes look good to me, but I'll defer to Ramon for a closer look.

tests/sdk/nodejs/metadata/step2/index.ts Show resolved Hide resolved
provider/pkg/provider/provider.go Outdated Show resolved Hide resolved
tests/sdk/nodejs/nodejs_test.go Show resolved Hide resolved
@EronWright EronWright enabled auto-merge (squash) September 21, 2023 18:39
@EronWright EronWright merged commit 550b206 into master Sep 21, 2023
18 checks passed
@EronWright EronWright deleted the eronwright/issue-2351 branch September 21, 2023 19:21
Copy link
Contributor

@rquitales rquitales left a comment

Choose a reason for hiding this comment

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

It looks like auto-merge didn't block until an approval was given. Regardless, the new changes LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants