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

Ensure loaded plugin config is saved to helm server. #6022

Merged
merged 1 commit into from
Feb 24, 2023

Conversation

absoludity
Copy link
Contributor

Description of the change

In #5339 we inadvertently switched from an assignment to a short variable declaration:

-		pluginConfig, err = common.ParsePluginConfig(pluginConfigPath)
+		pluginConfig, err := common.ParsePluginConfig(pluginConfigPath)

which meant that, in context, a new pluginConfig var was created in that scope, so that the actual pluginConfig var from the outside scope remained empty.

Benefits

Plugin config works again for the Helm plugin.

Possible drawbacks

Applicable issues

Additional information

I spent some time trying to write a test for this, but I could either extract the 5 lines to a separate function and test that or test the NewServer function itself. The latter is very difficult because it instantiates a bunch of network requests, and the former seems redundant since we already extract the functionality to the common.ParsePluginConfig() function... it's really just a golang gotcha that is important to be aware of: a := foo is equivalent to var a atype \n a = foo and so is always creating a new variable in that scope.

@netlify
Copy link

netlify bot commented Feb 24, 2023

Deploy Preview for kubeapps-dev canceled.

Name Link
🔨 Latest commit 03dd109
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/63f7ffc4524199000856ba59

@absoludity absoludity merged commit 56bc140 into main Feb 24, 2023
@absoludity absoludity deleted the 6020-chart-versions-config branch February 24, 2023 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[regression] Cannot control which chart versions are displayed
2 participants