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

fix CreateBeforeDestroy with datasources #9853

Merged
merged 2 commits into from
Nov 4, 2016
Merged

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Nov 3, 2016

The graph transformation we implement around create_before_destroy
need to re-order all resources that depend on the create_before_destroy
resource. Up until now, we've requires that users mark all of these
resources as create_before_destroy. Data soruces however don't have a
lifecycle block for create_before_destroy, and could not be marked this
way.

This PR checks each DestroyNode that doesn't implement CreateBeforeDestroy
for any ancestors that do implement CreateBeforeDestroy. If there are
any, we inherit the behavior and re-order the graph as such.

@jbardin jbardin added the core label Nov 3, 2016
@jbardin jbardin force-pushed the jbardin/cbd-datasource branch 2 times, most recently from 132a1bc to fafe737 Compare November 3, 2016 20:58
@jbardin jbardin changed the title [WIP] fix CreateBeforeDestroy with datasources fix CreateBeforeDestroy with datasources Nov 3, 2016
The graph transformation we implement around create_before_destroy
need to re-order all resources that depend on the create_before_destroy
resource. Up until now, we've requires that users mark all of these
resources as create_before_destroy. Data soruces however don't have a
lifecycle block for create_before_destroy, and could not be marked this
way.

This PR checks each DestroyNode that doesn't implement CreateBeforeDestroy
for any ancestors that do implement CreateBeforeDestroy. If there are
any, we inherit the behavior and re-order the graph as such.
Copy link
Contributor

@mitchellh mitchellh left a comment

Choose a reason for hiding this comment

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

Looks great.

One thing that is really critical with stuff like this is that we always pair it with a context test (apply, plan, etc. whatever it affects). This will allow us to move to new graphs more safely, which we do have planned. :)

Can you add a test to context_plan_test.go? I think this was affecting plan you said.

@jbardin jbardin force-pushed the jbardin/cbd-datasource branch from fafe737 to cf3a259 Compare November 3, 2016 21:23
This test will fail with a cycle before we check ancestors for
CreateBeforeDestroy.
@apparentlymart
Copy link
Contributor

Awesome to see this fixed! 😀

It looks like this would also address managed resources that depend on CBS resources, right? There is at least one issue out there for the cycles that causes, and some warnings about it in the docs for CBS.

@jbardin
Copy link
Member Author

jbardin commented Nov 4, 2016

@apparentlymart,

Yes, I think this should fix many (most, all??!) issues around cycles created from CBD resources. If this does work generally, we won't need the extra documentation and warnings about setting CBD for all dependent resources when you use it in a config, as TF should now properly re-order the the entire dependent destroy sequence.

I think there's probably a more precise way to do this after the CreateBeforeDestroy transform, by analyzing the subgraph cycles and only re-ordering the minimal set necessary, but this seems like a good first step and a more precise ordering may not be necessary.

@jbardin jbardin merged commit 07ec946 into master Nov 4, 2016
@jbardin jbardin deleted the jbardin/cbd-datasource branch November 4, 2016 13:44
@ghost
Copy link

ghost commented Apr 20, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants