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

Grandchild module orphans should be destroyed #2786

Merged
merged 5 commits into from
Jul 20, 2015

Conversation

mitchellh
Copy link
Contributor

Fixes #2770

This was an interesting bug found by @ryanking. Basically: when adding orphan modules to the graph, only direct descendants were added by inspecting the state. If the direct descendant has no state (no resources in the original config), then grandchildren will never get discovered and never added to the graph.

This PR mainly modifies State.ModuleOrphans to return orphans that have grandchildren even if they're not in the state.

This PR also contains a few bug fixes necessary to make this work otherwise, but the above is the meat of the change.

(Aside: we should really switch to a tree structure internally to make functions like ModuleOrphans more efficient than O(N), but... one day).

I was worried about the implications of deeply nested orphaned modules
in the parent commit, so I added a test. It's failing but not quite like
I expected it to. Perhaps I've uncovered an unrelated bug here?

/cc @mitchellh
@phinze
Copy link
Contributor

phinze commented Jul 20, 2015

Pushed a failing test - see commit message for details.

key := m.Path[len(m.Path)-2]
if _, ok := direct[key]; ok {
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wrote my failing test trying to explore the implications of this. I was worried that this would improperly skip great-grandchildren when the grandchild was orphaned (since the transform, and therefore this function only ever runs on the root path AFAICT).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I'll take a look. I thought of this but didn't write a test for it, my thought was that since we add the direct module orphan directly, this transform should be called recursively. But I'll take a look.

@mitchellh
Copy link
Contributor Author

Pushed a fix, it does get called recursively but there was an arithmetic error in there. :)

@phinze
Copy link
Contributor

phinze commented Jul 20, 2015

Bingo, LGTM if the tests pass

mitchellh added a commit that referenced this pull request Jul 20, 2015
Grandchild module orphans should be destroyed
@mitchellh mitchellh merged commit 853f4f2 into master Jul 20, 2015
@mitchellh mitchellh deleted the b-nested-module-orphans branch July 20, 2015 15:52
@ghost
Copy link

ghost commented May 1, 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 May 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to delete an entire module
3 participants