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

Set delete-before-replace for manually-named resources. #465

Merged
merged 3 commits into from
Aug 21, 2019

Conversation

pgavlin
Copy link
Member

@pgavlin pgavlin commented Aug 16, 2019

Determine if a resource is auto-named by checking its list of top-level
defaults for a property that was populated by a default value that is an
autoname. If the defaults list is missing, consider the resource
auto-named for better backwards compatibility.

These changes also fix a bug in the logic that determined the legacy
replaces list.

Fixes #383.

Determine if a resource is auto-named by checking its list of top-level
defaults for a property that was populated by a default value that is an
autoname. If the defaults list is missing, consider the resource
auto-named for better backwards compatibility.

These changes also fix a bug in the logic that determined the legacy
replaces list.

Fixes #383.
Copy link
Member

@lukehoban lukehoban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Would love to verify that deleteBeforeReplace: false will work as a way to override this if needed.

Copy link
Contributor

@stack72 stack72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pgavlin this is awesome :) Nice work!

@pgavlin
Copy link
Member Author

pgavlin commented Aug 19, 2019

Would love to verify that deleteBeforeReplace: false will work as a way to override this if needed.

As of this writing, that will not work. We will need to change the CLI in order to provide that override.

pgavlin added a commit to pulumi/pulumi that referenced this pull request Aug 20, 2019
With these changes, a user may explicitly set `deleteBeforeReplace` to
`false` in order to disable DBR behavior for a particular resource. This
is the SDK + CLI escape hatch for cases where the changes in
pulumi/pulumi-terraform#465 cause undesirable
behavior.
pgavlin added a commit to pulumi/pulumi that referenced this pull request Aug 20, 2019
With these changes, a user may explicitly set `deleteBeforeReplace` to
`false` in order to disable DBR behavior for a particular resource. This
is the SDK + CLI escape hatch for cases where the changes in
pulumi/pulumi-terraform#465 cause undesirable
behavior.
pgavlin added a commit to pulumi/pulumi that referenced this pull request Aug 20, 2019
With these changes, a user may explicitly set `deleteBeforeReplace` to
`false` in order to disable DBR behavior for a particular resource. This
is the SDK + CLI escape hatch for cases where the changes in
pulumi/pulumi-terraform#465 cause undesirable
behavior.
@pgavlin
Copy link
Member Author

pgavlin commented Aug 20, 2019

One thing I should point out: this will enable DBR for any resource that is not autonamed, which is subtly different than enabling DBR only for resources that are manually-named. With these changes, any resource that does not have an autonameable property will always be DBR'd. There are 276 such resources in the AWS provider at the moment; I haven't looked at the others.

The original code would have enabled DBR for any resource that is not
autonamed, which is subtly different than enabling DBR only for
resources that are manually-named. With those changes, any resource
that does not have an autonameable property would always have been
deleted before being replaced. This is probably not the correct default
for resources that do not have an autonamable property.

This commit changes the way we decide whether or not a resource's name
requires DBR: rather than looking for the presence of a default that
came from an autoname, the code now looks for an input property with a
manually-specified (i.e. non-default) value that _could_ have been
autonamed.
@pgavlin
Copy link
Member Author

pgavlin commented Aug 20, 2019

I have added a commit that changes the way we decide whether or not a resource's name requires DBR: rather than looking for the presence of a default that came from an autoname, the code now looks for an input property with a manually-specified (i.e. non-default) value that could have been autonamed.

@pgavlin
Copy link
Member Author

pgavlin commented Aug 21, 2019

I have manually validated that this works as expected with resources that are not auto-nameable (e.g. EC2 instances) as well as with resources that are. I have also validated that deleteBeforeReplace: false works as expected when using the latest dev versions of the CLI and SDK.

@pgavlin pgavlin merged commit 96182a4 into master Aug 21, 2019
@pulumi-bot pulumi-bot deleted the pgavlin/dbrNoAutoname branch August 21, 2019 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider setting deleteBeforeReplace by default when a resource is not autonamed
3 participants