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

Installing Helm v3 Chart resource causes errors #2638

Closed
scottslowe opened this issue Oct 25, 2023 · 1 comment · Fixed by #2655
Closed

Installing Helm v3 Chart resource causes errors #2638

scottslowe opened this issue Oct 25, 2023 · 1 comment · Fixed by #2655
Assignees
Labels
kind/bug Some behavior is incorrect or out of spec language/go resolution/fixed This issue was fixed
Milestone

Comments

@scottslowe
Copy link

What happened?

I was trying to use/update the kubernetes-go-helm-wordpress example with the Kubernetes v4 provider. I kept getting this error:

error: an unhandled error occurred: program exited with non-zero exit code: 2

    panic: interface conversion: interface {} is nil, not *v1.Service
    goroutine 119 [running]:
    main.main.func1.1({0x0?, 0x0?})
    	/Users/slowe/Work/Code/testarea/main.go:24 +0x5c

Through additional testing, @rquitales verified that this is broken if the chart is installed into the "default" namespace, and is broken in preview for all namespaces.

Example

  1. Stand up a Kubernetes cluster and obtain the Kubeconfig.
  2. Clone or copy down the kubernetes-go-helm-wordpress example with the Kubernetes v4 provider.
  3. With the Kubeconfig from step 1 active, run pulumi up. It should error during preview.

Output of pulumi about

CLI
Version      3.88.0
Go Version   go1.21.2
Go Compiler  gc

Plugins
NAME    VERSION
aws     6.5.0
awsx    2.0.2
docker  4.4.3
go      unknown

Host
OS       darwin
Version  13.5.2
Arch     arm64

Additional context

No response

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@scottslowe scottslowe added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team labels Oct 25, 2023
@rquitales rquitales self-assigned this Oct 25, 2023
@rquitales
Copy link
Member

We omit the namespace if it's default in the GetResource function:

if len(namespace) > 0 && namespace != "default" {
id = fmt.Sprintf("%s/%s", namespace, name)
}

We always need to include the namespace in generating the key. We should also double check the implementation for ConfigFile resources, and ensure it works as expected too!

@rquitales rquitales added language/go and removed needs-triage Needs attention from the triage team labels Oct 25, 2023
@rquitales rquitales added this to the 0.96 milestone Oct 25, 2023
@mikhailshilkov mikhailshilkov modified the milestones: 0.96, 0.97 Nov 23, 2023
rquitales added a commit that referenced this issue Dec 12, 2023
…mespace (#2655)

### Proposed changes

This pull request modifies the `GetResource` Go method for the Helm
Chart resource. Previously, when a resource was deployed to the default
namespace, the `default` namespace was always omitted from the resource
lookup key. However, in cases where the namespace is explicitly defined
in the Helm chart template, the `default` namespace was inadvertently
excluded from the resource lookup key despite it being needed. This
behavior in Helm is documented in the upstream issue:
helm/helm#3553.

**Changes Made:**
- Implemented a fallback mechanism to include the `default` namespace in
the resource lookup key when necessary for the `GetResource` method.

**Test Added:**
- Added a test (`ChartGetResource`) to verify the `GetResource` method
for both types of Helm charts—those with the explicitly defined default
namespace and those without.

**Verification:**
- Verified that the modified `GetResource` method successfully handles
scenarios where the namespace is explicitly defined in the Helm chart
template.
- Confirmed that the added test covers both types of Helm charts and
fails when the fallback logic is reverted.

### Related issues (optional)

Fixes: #2638
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Dec 12, 2023
rquitales added a commit that referenced this issue Dec 15, 2023
…mespace (#2655)

This pull request modifies the `GetResource` Go method for the Helm
Chart resource. Previously, when a resource was deployed to the default
namespace, the `default` namespace was always omitted from the resource
lookup key. However, in cases where the namespace is explicitly defined
in the Helm chart template, the `default` namespace was inadvertently
excluded from the resource lookup key despite it being needed. This
behavior in Helm is documented in the upstream issue:
helm/helm#3553.

**Changes Made:**
- Implemented a fallback mechanism to include the `default` namespace in
the resource lookup key when necessary for the `GetResource` method.

**Test Added:**
- Added a test (`ChartGetResource`) to verify the `GetResource` method
for both types of Helm charts—those with the explicitly defined default
namespace and those without.

**Verification:**
- Verified that the modified `GetResource` method successfully handles
scenarios where the namespace is explicitly defined in the Helm chart
template.
- Confirmed that the added test covers both types of Helm charts and
fails when the fallback logic is reverted.

Fixes: #2638
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Some behavior is incorrect or out of spec language/go resolution/fixed This issue was fixed
Projects
None yet
4 participants