Creating bool debug

[JaylonmcShan03]

Description

This PR refactors the handling of the debug mode in the Helm provider to use a boolean variable instead of repeated string comparisons.

Acceptance tests

Release Note

Release note for CHANGELOG:

...

References

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

[BBBmau]

I'm curious how the config overrides the environment variables if both cases set the variable to true.

1. Getting the HELM_DEBUG where it's "true" will mark `debug = true
2. Getting the configuration values will mark debug = true

Yet i don't see how we apply the override of config values instead of env variables if:

configuration values are set AND HELM_DEBUG envvar is set to "true"

[JaylonmcShan03]

the logic sets debug to true if either the environment variable HELM_DEBUG is "true" or if the configuration explicitly sets debug to true.

with my understanding this ensures that debugging is enabled if either source requests it.

[BBBmau]

In that case we should consider treating the two conditionals as one:

terraform-provider-helm/helm-framework/helm/provider.go

Lines 342 to 348 in 393479e

	debug := false
	if os.Getenv("HELM_DEBUG") == "true" {
	debug = true
	}
	if !config.Debug.IsNull() {
	debug = true
	}

can be switched to:

debug := false
 if os.Getenv("HELM_DEBUG") == "true" || !config.Debug.IsNull() {
 	debug = true 
}

[JaylonmcShan03]

Gotcha! Due to the previous confusion where I treated two conditions as one, it wasn't very clear what was going on. But these checks are self explanatory so I will update it!

[BBBmau]

At a second glance I'm wondering if it's right to use !config.Debug.IsNull()

Since from my understanding if a user sets debug = false it is technically not null anymore and would return true and mark debug = true despite debug being set to false in the config. Thoughts?

Edit: This might be better suited: https://pkg.go.dev/github.com/hashicorp/terraform-plugin-framework@v1.11.0/types/basetypes#BoolValue.ValueBool

[JaylonmcShan03]

Good catch, you are right if the user sets debug. = false, due to it check if its not null it was simply just override it to true. I believe in order to properly handle this scenario > we should check both the null state and the actual boolean value of the Debug field.

I believe it should look something like this >
`debug := false

if os.Getenv("HELM_DEBUG") == "true" {
	debug = true
}

if !config.Debug.IsNull() {
	debug = config.Debug.ValueBool()
}`

[BBBmau]

i believe we can still keep them as one condition since from the docs it says:

ValueBool returns the known bool value. If Bool is null or unknown, returns false.

So in other words it would return the "known bool value" so it would return true if debug = true and false if debug = false in config.

if the value is unknown or Null would just return false.

[BBBmau]

nice one liner! One last request

Here debug is initialized on line 342 but is first used here in the provider.go file:

terraform-provider-helm/helm-framework/helm/provider.go

Line 460 in 2eb62ef

settings.Debug = debug

It's best practice to include the variable initialization near the first call of the variable. In order to reduce the need to go searching for where the variable was first set.

[BBBmau]

nice red diff! 🎉