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

Fix webhook panic after refresh #87

Merged
merged 6 commits into from
Aug 18, 2022
Merged

Fix webhook panic after refresh #87

merged 6 commits into from
Aug 18, 2022

Conversation

mchaynes
Copy link
Contributor

@mchaynes mchaynes commented Aug 17, 2022

Description

Fixes #85

The reason the panic happened was due to a mismatch between the properties produced by Read and the properties expected by Diff. You can see the difference in output states in more detail below, but the TL;DR is that we were adding an extra object with the key __inputs in our output property map upon Create and Update. Read did not stuff values into the __inputs object after reading.

In order to simplify things, I lifted every key we previously stored in __inputs to the top level, while still handling the old state. So when users upgrade to this new version, on pulumi up or pulumi refresh, they will see a diff as if we are adding additional output properties, and removing old properties. We don't do this __inputs thing in any other providers (See pulumi-command for example)

Additionally, I upgraded examples/go.mod to go1.19 so that I could upgrade our pinned version of pulumi/pulumi/sdk and pulumi/pulumi/pkg.

State

Before This PR

Before Refresh:

"inputs": {
    "active": true,
    "displayName": "yaml-webhook",
    "organizationName": "service-provider-test-org",
    "payloadUrl": "https://google.com"
},
"outputs": {
    "__inputs": {
        "active": true,
        "displayName": "yaml-webhook",
        "organizationName": "service-provider-test-org",
        "payloadUrl": "https://google.com"
    },
    "name": "03dbae8b"
},

After Refresh

"inputs": {
    "__inputs": {
        "active": true,
        "displayName": "yaml-webhook",
        "organizationName": "service-provider-test-org",
        "payloadUrl": "https://google.com"
    },
    "name": "03dbae8b"
},
"outputs": {
    "active": true,
    "displayName": "yaml-webhook",
    "organizationName": "service-provider-test-org",
    "payloadUrl": "https://google.com",
    "secret": ""
},

After This PR

(Still handles old state where __inputs was defined)

"inputs": {
    "active": true,
    "displayName": "yaml-webhook",
    "organizationName": "service-provider-test-org",
    "payloadUrl": "https://google.com",
    "secret": ""
},
"outputs": {
    "active": true,
    "displayName": "yaml-webhook",
    "name": "03dbae8b",
    "organizationName": "service-provider-test-org",
    "payloadUrl": "https://google.com"
},

Testing

  • Added new test cases to check that refresh works in nodejs and yaml.

Manual verification of upgrade

v0.1.6 --> PR version, with refresh
  1. Verified installed v0.1.6 version of the pulumi-service provider.
  2. Ran pulumi up in examples/yaml-webhooks.
  3. Ran pulumi refresh
  4. Upgraded pulumi-service provider to PR version.
  5. Ran pulumi up
  6. Observed pulumi up ran successfully and lifted values from __inputs to top-level outputs
v0.1.6 --> PR version, without refresh on v0.1.6
  1. Verified installed v0.1.6 version of the pulumi-service provider.
  2. Ran pulumi up in examples/yaml-webhooks.
  3. Upgraded pulumi-service provider to PR version.
  4. Ran pulumi up
  5. Observed pulumi up ran successfully and lifted values from __inputs to top-level outputs

@mchaynes mchaynes requested review from blampe and a team and removed request for blampe August 17, 2022 20:56
Copy link
Contributor

@blampe blampe left a comment

Choose a reason for hiding this comment

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

Ah I figured the __inputs was some Pulumi thing, I guess not! Nice cleanup.

@mchaynes mchaynes merged commit 9e1069e into main Aug 18, 2022
@mchaynes mchaynes deleted the myles/webhook-panic branch August 18, 2022 01:31
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.

Error when refreshing stack that includes a Pulumi webhook
2 participants