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(bridge): default value initialization for ExactlyOneOf fields #693

Merged
merged 1 commit into from Dec 16, 2022

Conversation

AaronFriel
Copy link
Member

@AaronFriel AaronFriel commented Dec 15, 2022

Fixes pulumi/pulumi-cloudflare#306

In the Cloudflare provider, three fields are set to ExactlyOneOf: api_token, api_key, and api_user_service_key. The subroutine in applyDefaults that iterates over TF schema and applies default values would return on line 705 when name == "api_token" because:

sch.ExactlyOneOf() returned []string{"api_token", "api_key", and "api_user_service_key"}, and then the ExactlyOneOf check in the loop to set defaults (lines 701-708) would evaluate like so:

676:  tfs.Range(func(name string, sch shim.Schema) bool {
      // in the loop iteration where name == "api_token"

// ...

701:  for _, exactlyOneOfName := range sch.ExactlyOneOf() {
        // exactlyOneOfName would eventually equal "api_token"
702:    if exactlyOneSchema, exists := tfs.GetOk(exactlyOneOfName); exists {
          // exactlyOneSchema == api_token's TF schema
          // exists == true
703:      dv, _ := exactlyOneSchema.DefaultValue()
          // dv := value of CLOUDFLARE_API_TOKEN env var
704:      if dv != nil {
            // dv != nil == true
705:        return true
          }
        }
      }

This caused any field with ExactlyOneOf and a default value func to skip default initialization: it would always find itself in this loop as the "other" key, discover that other key had a default value, and then exit the loop.

@github-actions
Copy link

Diff for pulumi-random with merge commit 2d11808

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 2d11808

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 2d11808

@github-actions
Copy link

Diff for pulumi-azure with merge commit 2d11808

@t0yv0 t0yv0 self-requested a review December 15, 2022 01:21
@github-actions
Copy link

Diff for pulumi-aws with merge commit 2d11808

@squaremo
Copy link

  1. I <3 the detailed explanation for what this fixes. Can we have the explanation in the commit message please, so that it shows up outside GitHub
  2. There are unit tests for this file, but evidently they didn't cover this situation -- how about a unit test to guard against regressions

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 5310423

@github-actions
Copy link

Diff for pulumi-random with merge commit 5310423

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 5310423

@github-actions
Copy link

Diff for pulumi-azure with merge commit 5310423

Fixes pulumi/pulumi-cloudflare#306

In the Cloudflare provider, three fields are set to ExactlyOneOf: `api_token`,
`api_key`, and `api_user_service_key`. The subroutine in `applyDefaults` that
iterates over TF schema and applies default values would return on line 705 when
`name == "api_token"` because:

`sch.ExactlyOneOf()` returned `[]string{"api_token", "api_key", and
"api_user_service_key"}`, and then the ExactlyOneOf check in the loop to set
defaults (lines 701-708) would evaluate like so:

```go
676:  tfs.Range(func(name string, sch shim.Schema) bool {
      // in the loop iteration where name == "api_token"

// ...

701:  for _, exactlyOneOfName := range sch.ExactlyOneOf() {
        // exactlyOneOfName would eventually equal "api_token"
702:    if exactlyOneSchema, exists := tfs.GetOk(exactlyOneOfName); exists {
          // exactlyOneSchema == api_token's TF schema
          // exists == true
703:      dv, _ := exactlyOneSchema.DefaultValue()
          // dv := value of CLOUDFLARE_API_TOKEN env var
704:      if dv != nil {
            // dv != nil == true
705:        return true
          }
        }
      }
```

This caused any field with `ExactlyOneOf` and a default value func to skip
default initialization: it would always find itself in this loop as the "other"
key, discover that other key had a default value, and then exit the loop.
@github-actions
Copy link

Diff for pulumi-random with merge commit d3623d1

@github-actions
Copy link

Diff for pulumi-azuread with merge commit d3623d1

@github-actions
Copy link

Diff for pulumi-aws with merge commit 5310423

@github-actions
Copy link

Diff for pulumi-gcp with merge commit d3623d1

@github-actions
Copy link

Diff for pulumi-azure with merge commit d3623d1

@github-actions
Copy link

Diff for pulumi-aws with merge commit d3623d1

@AaronFriel
Copy link
Member Author

@t0yv0 @squaremo updated with a test and verified the test passed due to the code change. 👍

Good call on the test, it turns out there was an issue with the harness.

@squaremo
Copy link

updated with a test and verified the test passed due to the code change

💯 thank you!

@AaronFriel AaronFriel merged commit 56d0534 into master Dec 16, 2022
@AaronFriel AaronFriel deleted the friel/fix-one-of branch December 16, 2022 16:46
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.

v4.13.0 no longer accepts API token from env var?
3 participants