Fix chef provisioner validateFn

[jeremiahsnapp]

Correctly validate Chef provisioner's use_policyfile field even if its value is a string type.

This fixes #16655

[svanharmelen]

Thanks for your PR @jeremiahsnapp! I see what you are trying to do, but I wonder if this isn't actually a bug in Terraform core instead.

@apparentlymart the schema clearly specifies the use_policyfile field as being a schema.TypeBool, so shouldn't TF core return an error on this instead?

[jeremiahsnapp]

Thanks for looking at this @svanharmelen and @apparentlymart. I'm new to the terraform code so I'm not certain if their is a bug in the core code. I will say that the Booleans docs gives me the impression that using string values for a boolean variable is preferred and that TF should automatically convert the value internally to a boolean as needed.

The chef provisioner's validateFn function is being passed a terraform.ResourceConfig. I believe TF's core ResourceConfig.Get does not do the automatic conversion from string to boolean because it does not take into account the data's schema.

Contrast this with how the chef provisioner's decodeConfig function is passed a schema.ResourceData. I believe the ResourceData.Get does do the automatic conversion from string to boolean because it does take into account the data's schema.

So while the schema.ResourceData part of the code is auto converting the variable properly the terraform.ResourceConfig part is not. If that is intentional by design then I think my PR is required to fix the chef provisioner. If that is not the intended design then I agree that the bug is somewhere in TF core. Hopefully one of you can help isolate this issue better.

Thanks!

[svanharmelen]

Your absolutely right! I didn't look good enough and responded to quickly I guess. So your fix does indeed seem to be needed 👍

I do have a suggestion for your switch statement. Most important changes are adding a default case and return when getting an error.

To me it feels a bit wierd/wrong to check config fields that are dependent on a field we already know isn't used correctly.

Let me know your thoughts! And thanks again for your elaborate update and the PR!

[svanharmelen]

May I suggest a small rewrite of your switch statement to make it just a bit more idiomatic Go?

switch usePolicyFileRaw := usePolicyFileRaw.(type) {
  case bool:
    usePolicyFile = usePolicyFileRaw
  case string:
    usePolicyFileBool, err := strconv.ParseBool(usePolicyFileRaw)
    if err != nil {
      return append(es, errors.New("\"use_policyfile\" must be a boolean"))
    }
    usePolicyFile = usePolicyFileBool
  default:
    return append(es, errors.New("\"use_policyfile\" must be a boolean")) 
}

[jeremiahsnapp]

Thanks a lot for the idiomatic Go advice @svanharmelen. I like it a lot. I only made a minor change to your return statements so they return the correct number of arguments. Thanks!

[svanharmelen]

Again thanks for the PR and your inputs! LGTM! 🎉

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.