-
Notifications
You must be signed in to change notification settings - Fork 115
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
Fix error for helm.Release previews with computed values #1760
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1148,12 +1148,6 @@ func (k *kubeProvider) Check(ctx context.Context, req *pulumirpc.CheckRequest) ( | |
// | ||
|
||
urn := resource.URN(req.GetUrn()) | ||
if isHelmRelease(urn) { | ||
if !k.clusterUnreachable { | ||
return k.helmReleaseProvider.Check(ctx, req, !k.suppressHelmReleaseBetaWarning) | ||
} | ||
return nil, fmt.Errorf("can't use Helm Release with unreachable cluster. Reason: %q", k.clusterUnreachableReason) | ||
} | ||
|
||
// Utilities for determining whether a resource's GVK exists. | ||
gvkExists := func(gvk schema.GroupVersionKind) bool { | ||
|
@@ -1218,6 +1212,13 @@ func (k *kubeProvider) Check(ctx context.Context, req *pulumirpc.CheckRequest) ( | |
} | ||
newInputs = annotatedInputs | ||
|
||
if isHelmRelease(urn) && !hasComputedValue(newInputs) { | ||
if !k.clusterUnreachable { | ||
return k.helmReleaseProvider.Check(ctx, req, !k.suppressHelmReleaseBetaWarning) | ||
} | ||
return nil, fmt.Errorf("can't use Helm Release with unreachable cluster. Reason: %q", k.clusterUnreachableReason) | ||
} | ||
|
||
// Adopt name from old object if appropriate. | ||
// | ||
// If the user HAS NOT assigned a name in the new inputs, we autoname it and mark the object as | ||
|
@@ -1391,12 +1392,6 @@ func (k *kubeProvider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*p | |
// | ||
|
||
urn := resource.URN(req.GetUrn()) | ||
if isHelmRelease(urn) { | ||
if !k.clusterUnreachable { | ||
return k.helmReleaseProvider.Diff(ctx, req) | ||
} | ||
return nil, fmt.Errorf("can't use Helm Release with unreachable cluster. Reason: %q", k.clusterUnreachableReason) | ||
} | ||
|
||
label := fmt.Sprintf("%s.Diff(%s)", k.label(), urn) | ||
logger.V(9).Infof("%s executing", label) | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
return nil, fmt.Errorf("can't use Helm Release with unreachable cluster. Reason: %q", k.clusterUnreachableReason) | ||
} | ||
|
||
namespacedKind, err := clients.IsNamespacedKind(gvk, k.clientSet) | ||
if err != nil { | ||
if clients.IsNoNamespaceInfoErr(err) { | ||
|
@@ -1598,7 +1600,8 @@ func (k *kubeProvider) Create( | |
// comments in those methods for details. | ||
// | ||
urn := resource.URN(req.GetUrn()) | ||
if isHelmRelease(urn) { | ||
|
||
if isHelmRelease(urn) && !req.GetPreview() { | ||
if !k.clusterUnreachable { | ||
return k.helmReleaseProvider.Create(ctx, req) | ||
} | ||
|
@@ -2063,15 +2066,6 @@ func (k *kubeProvider) Update( | |
} | ||
newInputs := propMapToUnstructured(newResInputs) | ||
|
||
if isHelmRelease(urn) { | ||
if !k.clusterUnreachable { | ||
return k.helmReleaseProvider.Update(ctx, req) | ||
} | ||
return nil, fmt.Errorf("can't update Helm Release with unreachable cluster. Reason: %q", k.clusterUnreachableReason) | ||
} | ||
// Ignore old state; we'll get it from Kubernetes later. | ||
oldInputs, _ := parseCheckpointObject(oldState) | ||
|
||
// If this is a preview and the input values contain unknowns, return them as-is. This is compatible with | ||
// prior behavior implemented by the Pulumi engine. Similarly, if the server does not support server-side | ||
// dry run, return the inputs as-is. | ||
|
@@ -2082,6 +2076,15 @@ func (k *kubeProvider) Update( | |
return &pulumirpc.UpdateResponse{Properties: req.News}, nil | ||
} | ||
|
||
if isHelmRelease(urn) { | ||
if !k.clusterUnreachable { | ||
lblackstone marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return k.helmReleaseProvider.Update(ctx, req) | ||
} | ||
return nil, fmt.Errorf("can't update Helm Release with unreachable cluster. Reason: %q", k.clusterUnreachableReason) | ||
} | ||
// Ignore old state; we'll get it from Kubernetes later. | ||
oldInputs, _ := parseCheckpointObject(oldState) | ||
|
||
annotatedInputs, err := withLastAppliedConfig(newInputs) | ||
if err != nil { | ||
return nil, pkgerrors.Wrapf( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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#L261There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I think we can remove all references to keepUnkowns in helm_release.