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.

Sign up for GitHub

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

Creating bool debug #1469

Merged
merged 9 commits into from
Aug 28, 2024
Merged

Creating bool debug #1469

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2ef9ee6
Fix acronyms in struct field names in kubeConfig.go and provider.go
JaylonmcShan03 Aug 27, 2024
2e92927
Fixed acronym in kubeConfig that wasnt pushed in previous pr
JaylonmcShan03 Aug 28, 2024
c98e6f9
Merge branch 'helm-framework' of https://github.com/hashicorp/terrafo…
JaylonmcShan03 Aug 28, 2024
4d30008
Refactored debug handling to use a boolean variable
JaylonmcShan03 Aug 28, 2024
393479e
removing unnessary comment
JaylonmcShan03 Aug 28, 2024
bc9346b
Updating the debug two conditionals into one
JaylonmcShan03 Aug 28, 2024
b440ce0
Fixing edge case logic
JaylonmcShan03 Aug 28, 2024
2eb62ef
Simpliyfing if statements
JaylonmcShan03 Aug 28, 2024
28a4c3a
Fixing location of debug variable
JaylonmcShan03 Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions helm-framework/helm/kubeConfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,8 @@ func (m *Meta) NewKubeConfig(ctx context.Context, namespace string) (*KubeConfig
"tlsServerName": overrides.ClusterInfo.TLSServerName,
})
}
if !kubernetesConfig.ClusterCaCertificate.IsNull() {
overrides.ClusterInfo.CertificateAuthorityData = []byte(kubernetesConfig.ClusterCaCertificate.ValueString())
if !kubernetesConfig.ClusterCACertificate.IsNull() {
overrides.ClusterInfo.CertificateAuthorityData = []byte(kubernetesConfig.ClusterCACertificate.ValueString())
fmt.Println("Setting cluster CA certificate")
tflog.Debug(ctx, "Setting cluster CA certificate")
}
Expand Down
12 changes: 8 additions & 4 deletions helm-framework/helm/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,6 @@ func execSchema() map[string]schema.Attribute {

// Setting up the provider, anything we need to get the provider running, probbaly authentication. like the api
func (p *HelmProvider) Configure(ctx context.Context, req provider.ConfigureRequest, resp *provider.ConfigureResponse) {
debug := os.Getenv("HELM_DEBUG")
pluginsPath := os.Getenv("HELM_PLUGINS_PATH")
registryConfigPath := os.Getenv("HELM_REGISTRY_CONFIG_PATH")
repositoryConfigPath := os.Getenv("HELM_REPOSITORY_CONFIG_PATH")
Expand Down Expand Up @@ -339,9 +338,14 @@ func (p *HelmProvider) Configure(ctx context.Context, req provider.ConfigureRequ
if resp.Diagnostics.HasError() {
return
}

debug := false
if os.Getenv("HELM_DEBUG") == "true" {
debug = true
}
// Override environment variables if the configuration values are provided
Copy link
Contributor

Choose a reason for hiding this comment

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

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

@BBBmau BBBmau Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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/[email protected]/types/basetypes#BoolValue.ValueBool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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()
}`

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if !config.Debug.IsNull() {
debug = fmt.Sprintf("%t", config.Debug.ValueBool())
debug = true
}
if !config.PluginsPath.IsNull() {
pluginsPath = config.PluginsPath.ValueString()
Expand Down Expand Up @@ -459,7 +463,7 @@ func (p *HelmProvider) Configure(ctx context.Context, req provider.ConfigureRequ
"config": config,
})
settings := cli.New()
settings.Debug = debug == "true"
settings.Debug = debug
if pluginsPath != "" {
settings.PluginsDirectory = pluginsPath
}
Expand Down Expand Up @@ -579,7 +583,7 @@ func (p *HelmProvider) Configure(ctx context.Context, req provider.ConfigureRequ

meta := &Meta{
Data: &HelmProviderModel{
Debug: types.BoolValue(debug == "true"),
BBBmau marked this conversation as resolved.
Show resolved Hide resolved
Debug: types.BoolValue(debug),
PluginsPath: types.StringValue(pluginsPath),
RegistryConfigPath: types.StringValue(registryConfigPath),
RepositoryConfigPath: types.StringValue(repositoryConfigPath),
Expand Down
Loading