Fix error for helm.Release previews with computed values #1760
Conversation
The helm.Release resource was not checking for computed values in the configuration during preview, which led to various failures in code that expects only resolved values.
lblackstone
force-pushed
the
lblackstone/helm-release-preview
branch
from
8de76db
to
aacab02
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
2 similar comments
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
| @@ -1218,6 +1212,13 @@ func (k *kubeProvider) Check(ctx context.Context, req *pulumirpc.CheckRequest) ( | |||
| } | |||
| newInputs = annotatedInputs | |||
|
|
|||
| if isHelmRelease(urn) && !hasComputedValue(newInputs) { | |||
There was a problem hiding this comment.
How disruptive is that? Say, we have 9 plain values and 1 computed value - wouldn't we still want to run Check for those 9 plain values? Should we teach helmReleaseProvider to work with computed values instead?
There was a problem hiding this comment.
The helm release check attempts to unmarshal the arguments, which doesn't work with computed values. The rest of the check after unmarshaling involves talking to the helm client, so it's not possible without resolved values. I can't think of a case where we could do useful checking (specific to Release) with unresolved values, so I think it's fine to only do the checks at the k8s provider layer.
To clarify, all of the normal Check logic still runs at the k8s provider layer, so the preview would look something like this:
+ pulumi:pulumi:Stack: (create)
[urn=urn:pulumi:dev::pulumi-k8s-test::pulumi:pulumi:Stack::pulumi-k8s-test-dev]
+ random:index/randomPet:RandomPet: (create)
[urn=urn:pulumi:dev::pulumi-k8s-test::random:index/randomPet:RandomPet::test]
[provider=urn:pulumi:dev::pulumi-k8s-test::pulumi:providers:random::default_4_2_0::04da6b54-80e4-46f7-96ec-b56ff0331ba9]
length : 2
separator : "-"
+ kubernetes:core/v1:Namespace: (create)
[urn=urn:pulumi:dev::pulumi-k8s-test::kubernetes:core/v1:Namespace::test]
[provider=urn:pulumi:dev::pulumi-k8s-test::pulumi:providers:kubernetes::default_3_9_0_alpha_1633622661_9bc68e49_dirty::04da6b54-80e4-46f7-96ec-b56ff0331ba9]
apiVersion: "v1"
kind : "Namespace"
metadata : {
labels: {
app.kubernetes.io/managed-by: "pulumi"
}
name : output<string>
}
+ kubernetes:helm.sh/v3:Release: (create)
[urn=urn:pulumi:dev::pulumi-k8s-test::kubernetes:helm.sh/v3:Release::nginx]
[provider=urn:pulumi:dev::pulumi-k8s-test::pulumi:providers:kubernetes::default_3_9_0_alpha_1633622661_9bc68e49_dirty::04da6b54-80e4-46f7-96ec-b56ff0331ba9]
chart : "nginx"
compat : "true"
metadata : {
annotations: {
pulumi.com/autonamed: "true"
}
labels : {
app.kubernetes.io/managed-by: "pulumi"
}
name : "nginx-wj35smcw"
}
namespace : output<string>
repositoryOpts: {
repo: "https://charts.bitnami.com/bitnami"
}
There was a problem hiding this comment.
I see, thank you. Should we remove KeepUnknowns: true from here then? https://github.com/pulumi/pulumi-kubernetes/blob/master/provider/pkg/provider/helm_release.go#L261
There was a problem hiding this comment.
Good point. I think we can remove all references to keepUnkowns in helm_release.
| @@ -1431,6 +1426,13 @@ func (k *kubeProvider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*p | |||
| return nil, err | |||
| } | |||
|
|
|||
| if isHelmRelease(urn) && !hasComputedValue(newInputs) { | |||
| if !k.clusterUnreachable { | |||
| return k.helmReleaseProvider.Diff(ctx, req) | |||
There was a problem hiding this comment.
Same question for Diff I guess. How does the helm-release diff differ from the default diff and are we ready to lose that for any computed value?
There was a problem hiding this comment.
@viveklak can verify, but I don't think it will be possible to get a more detailed Release diff with computed values since it requires talking to Helm. We still get all the normal provider diffing, which shows client-side config changes.
There was a problem hiding this comment.
Yeah unfortunately we need to rely on helm's ability to handle the templated values which won't be possible with computed values. I believe this is the same experience with helm charts as well.
Does the PR have any schema changes?Looking good! No breaking changes found. |
|
A relevant follow-up: #1953 |
Proposed changes
The helm.Release resource was not checking for computed
values in the configuration during preview, which led to
various failures in code that expects only resolved values.
Related issues (optional)
Fix #1725