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

core: respect roots in CBD transform #1953

Merged
merged 1 commit into from
May 13, 2015
Merged

core: respect roots in CBD transform #1953

merged 1 commit into from
May 13, 2015

Conversation

phinze
Copy link
Contributor

@phinze phinze commented May 13, 2015

refs #1903

Details in code comment.


// If the graph has a "root" node (one added by a RootTransformer and not
// just a resource that happens to have no ancestors), we don't want to
// add any edges to it, because then it ceases to be a root.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah this makes a lot of sense. We now do a root transformer potentially before the destroy transform which never happened before. I see how this happened.

@mitchellh
Copy link
Contributor

LGTM

Because CBD now runs after a RootTransformer, it's now operating on a
graph that _may_ have had a graphNodeRoot added to it (a noop node whose
only purpose is to be a root).

CBD includes a step that tells the destroy node to depend on any parents
of the create node. When one of those parents was "root", this was
causing the destroy node to depend on "root", making it cease to be an
actual root node.

Because graphNodeRoot is a singleton, the follow-up RootTransformer was
not sufficient to slap another root on top - it wasn't being seen as a
fresh node, so edges were just accumulating, and we ended up in a state
with "no roots".

refs #1903 (not sure if this will fix all the "no root found" cases, or
just the one I bumped into)
phinze added a commit that referenced this pull request May 13, 2015
core: respect roots in CBD transform
@phinze phinze merged commit c435e3e into master May 13, 2015
@phinze phinze deleted the b-no-roots-found branch May 13, 2015 22:57
@ghost
Copy link

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

Successfully merging this pull request may close these issues.

2 participants