-
Notifications
You must be signed in to change notification settings - Fork 162
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
Suppress undefined to default diffs on aws.autoscaling.Group #4510
Conversation
Fix a regression in the import experience on aws.autoscaling.Group. Specifically the conflict between undefined and default values that `pulumi import` used to detect is no longer a conflict by automatically injecting DiffSuppressFunc for the relevant properties. Fixes #4457
t.Logf("workdir = %s", workdir) | ||
importResult := execPulumi(t, ptest, workdir, "import", "aws:autoscaling/group:Group", "newag", autoGroupName, "--yes") | ||
|
||
t.Logf("Editing the program to add the code recommended by import") |
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.
AFAIK we have no precedent of testing this, throwing in some automation, can generalize later if this is important. I have a suspicion that the issue generalizes to many resources in the ecosystem as TF Read() is not guaranteed to apply defaults.
Does the PR have any schema changes?Looking good! No breaking changes found. Maintainer note: consult the runbook for dealing with any breaking changes. |
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.
Have we considered using a TransformOutputs
hook instead? That could be used to act on the import output, so that the fixup has to happen only once after the state is imported/
I was a little surprised but TransformOutputs hook works well. It is able to eliminate the default values from the printed imported program which is good, and it also fixes the issue. The ability to be certain about undefined vs null vs empty is much appreciated. I think it also bypasses the problem with hypothetical defaults change. |
I've tested refresh and import_ option manually, seem to work as well for this with these changes. |
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.
LGTM
} | ||
} | ||
defaults := map[string]resource.PropertyValue{ | ||
"forceDelete": defaultValue("force_delete"), |
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.
Can we detect the props that need the defaults to be fixed? This would help us generalize this and also make maintenance a bit easier (no need to play catchup with upstream).
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 think this works so well we should maybe change the bridge default behavior here in pulumi/pulumi-terraform-bridge#2436 so that's how it works by default without the custom code.
This PR has been shipped in release v6.53.0. |
Fix a regression in the import experience on aws.autoscaling.Group. Specifically the conflict between undefined and default values that
pulumi import
used to detect is no longer a conflict by automatically injecting DiffSuppressFunc for the relevant properties.Fixes #4457