core: refactoring.ImpliedMoveStatements replaces NodeCountBoundary

Going back a long time we've had a special magic behavior which tries to
recognize a situation where a module author either added or removed the
"count" argument from a resource that already has instances, and to
silently rename the zeroth or no-key instance so that we don't plan to
destroy and recreate the associated object.

Now we have a more general idea of "move statements", and specifically
the idea of "implied" move statements which replicates the same heuristic
we used to use for this behavior, we can treat this magic renaming rule as
just another "move statement", special only in that Terraform generates it
automatically rather than it being written out explicitly in the
configuration.

In return for wiring that in, we can now remove altogether the
NodeCountBoundary graph node type and its associated graph transformer,
CountBoundaryTransformer. We handle moves as a preprocessing step before
building the plan graph, so we no longer need to include any special nodes
in the graph to deal with that situation.

The test updates here are mainly for the graph builders themselves, to
acknowledge that indeed we're no longer inserting the NodeCountBoundary
vertices. The vertices that NodeCountBoundary previously depended on now
become dependencies of the special "root" vertex, although in many cases
here we don't see that explicitly because of the transitive reduction
algorithm, which notices when there's already an equivalent indirect
dependency chain and removes the redundant edge.

We already have plenty of test coverage for these "count boundary" cases
in the context tests whose names start with TestContext2Plan_count and
TestContext2Apply_resourceCount, all of which continued to pass here
without any modification and so are not visible in the diff. The test
functions particularly relevant to this situation are:
 - TestContext2Plan_countIncreaseFromNotSet
 - TestContext2Plan_countDecreaseToOne
 - TestContext2Plan_countOneIndex
 - TestContext2Apply_countDecreaseToOneCorrupted

The last of those in particular deals with the situation where we have
both a no-key instance _and_ a zero-key instance in the prior state, which
is interesting here because to exercises an intentional interaction
between refactoring.ImpliedMoveStatements and refactoring.ApplyMoves,
where we intentionally generate an implied move statement that produces
a collision and then expect ApplyMoves to deal with it in the same way as
it would deal with all other collisions, and thus ensure we handle both
the explicit and implied collisions in the same way.

This does affect some UI-level tests, because a nice side-effect of this
new treatment of this old feature is that we can now report explicitly
in the UI that we're assigning new addresses to these objects, whereas
before we just said nothing and hoped the user would just guess what had
happened and why they therefore weren't seeing a diff.

The backend/local plan tests actually had a pre-existing bug where they
were using a state with a different instance key than the config called
for but getting away with it because we'd previously silently fix it up.
That's still fixed up, but now done with an explicit mention in the UI
and so I made the state consistent with the configuration here so that the
tests would be able to recognize _real_ differences where present, as
opposed to the errant difference caused by that inconsistency.
This commit is contained in:
Martin Atkins 2021-09-17 15:32:32 -07:00
parent ee9e346039
commit f0034beb33
14 changed files with 65 additions and 344 deletions

View File

@ -746,7 +746,7 @@ func testPlanState() *states.State {
Mode: addrs.ManagedResourceMode,
Type: "test_instance",
Name: "foo",
}.Instance(addrs.IntKey(0)),
}.Instance(addrs.NoKey),
&states.ResourceInstanceObjectSrc{
Status: states.ObjectReady,
AttrsJSON: []byte(`{

View File

@ -47,22 +47,26 @@
},
"resource_drift": [
{
"address": "test_instance.test",
"address": "test_instance.test[0]",
"mode": "managed",
"type": "test_instance",
"provider_name": "registry.terraform.io/hashicorp/test",
"name": "test",
"index": 0,
"change": {
"actions": [
"delete"
"no-op"
],
"before": {
"ami": "bar",
"id": "placeholder"
},
"after": null,
"after": {
"ami": "bar",
"id": "placeholder"
},
"before_sensitive": {},
"after_sensitive": false,
"after_sensitive": {},
"after_unknown": {}
}
}

View File

@ -248,60 +248,6 @@ func (s *SyncState) RemoveResourceIfEmpty(addr addrs.AbsResource) bool {
return true
}
// MaybeFixUpResourceInstanceAddressForCount deals with the situation where a
// resource has changed from having "count" set to not set, or vice-versa, and
// so we need to rename the zeroth instance key to no key at all, or vice-versa.
//
// Set countEnabled to true if the resource has count set in its new
// configuration, or false if it does not.
//
// The state is modified in-place if necessary, moving a resource instance
// between the two addresses. The return value is true if a change was made,
// and false otherwise.
func (s *SyncState) MaybeFixUpResourceInstanceAddressForCount(addr addrs.ConfigResource, countEnabled bool) bool {
s.lock.Lock()
defer s.lock.Unlock()
// get all modules instances that may match this state
modules := s.state.ModuleInstances(addr.Module)
if len(modules) == 0 {
return false
}
changed := false
for _, ms := range modules {
relAddr := addr.Resource
rs := ms.Resource(relAddr)
if rs == nil {
continue
}
huntKey := addrs.NoKey
replaceKey := addrs.InstanceKey(addrs.IntKey(0))
if !countEnabled {
huntKey, replaceKey = replaceKey, huntKey
}
is, exists := rs.Instances[huntKey]
if !exists {
continue
}
if _, exists := rs.Instances[replaceKey]; exists {
// If the replacement key also exists then we'll do nothing and keep both.
continue
}
// If we get here then we need to "rename" from hunt to replace
rs.Instances[replaceKey] = is
delete(rs.Instances, huntKey)
changed = true
}
return changed
}
// SetResourceInstanceCurrent saves the given instance object as the current
// generation of the resource instance with the given address, simultaneously
// updating the recorded provider configuration address, dependencies, and

View File

@ -2073,9 +2073,22 @@ func TestContext2Apply_countDecreaseToOneX(t *testing.T) {
// https://github.com/PeoplePerHour/terraform/pull/11
//
// This tests a case where both a "resource" and "resource.0" are in
// the state file, which apparently is a reasonable backwards compatibility
// concern found in the above 3rd party repo.
// This tests a rare but possible situation where we have both a no-key and
// a zero-key instance of the same resource in the configuration when we
// disable count.
//
// The main way to get here is for a provider to fail to destroy the zero-key
// instance but succeed in creating the no-key instance, since those two
// can typically happen concurrently. There are various other ways to get here
// that might be considered user error, such as using "terraform state mv"
// to create a strange combination of different key types on the same resource.
//
// This test indirectly exercises an intentional interaction between
// refactoring.ImpliedMoveStatements and refactoring.ApplyMoves: we'll first
// generate an implied move statement from aws_instance.foo[0] to
// aws_instance.foo, but then refactoring.ApplyMoves should notice that and
// ignore the statement, in the same way as it would if an explicit move
// statement specified the same situation.
func TestContext2Apply_countDecreaseToOneCorrupted(t *testing.T) {
m := testModule(t, "apply-count-dec-one")
p := testProvider("aws")

View File

@ -297,7 +297,14 @@ func (c *Context) destroyPlan(config *configs.Config, prevRunState *states.State
}
func (c *Context) prePlanFindAndApplyMoves(config *configs.Config, prevRunState *states.State, targets []addrs.Targetable) ([]refactoring.MoveStatement, map[addrs.UniqueKey]refactoring.MoveResult) {
moveStmts := refactoring.FindMoveStatements(config)
explicitMoveStmts := refactoring.FindMoveStatements(config)
implicitMoveStmts := refactoring.ImpliedMoveStatements(config, prevRunState, explicitMoveStmts)
var moveStmts []refactoring.MoveStatement
if stmtsLen := len(explicitMoveStmts) + len(implicitMoveStmts); stmtsLen > 0 {
moveStmts = make([]refactoring.MoveStatement, 0, stmtsLen)
moveStmts = append(moveStmts, explicitMoveStmts...)
moveStmts = append(moveStmts, implicitMoveStmts...)
}
moveResults := refactoring.ApplyMoves(moveStmts, prevRunState)
return moveStmts, moveResults
}

View File

@ -2,10 +2,8 @@ package terraform
import (
"fmt"
"log"
"github.com/hashicorp/hcl/v2"
"github.com/hashicorp/terraform/internal/addrs"
"github.com/hashicorp/terraform/internal/tfdiags"
"github.com/zclconf/go-cty/cty"
"github.com/zclconf/go-cty/cty/gocty"
@ -101,32 +99,3 @@ func evaluateCountExpressionValue(expr hcl.Expression, ctx EvalContext) (cty.Val
return countVal, diags
}
// fixResourceCountSetTransition is a helper function to fix up the state when a
// resource transitions its "count" from being set to unset or vice-versa,
// treating a 0-key and a no-key instance as aliases for one another across
// the transition.
//
// The correct time to call this function is in the DynamicExpand method for
// a node representing a resource, just after evaluating the count with
// evaluateCountExpression, and before any other analysis of the
// state such as orphan detection.
//
// This function calls methods on the given EvalContext to update the current
// state in-place, if necessary. It is a no-op if there is no count transition
// taking place.
//
// Since the state is modified in-place, this function must take a writer lock
// on the state. The caller must therefore not also be holding a state lock,
// or this function will block forever awaiting the lock.
func fixResourceCountSetTransition(ctx EvalContext, addr addrs.ConfigResource, countEnabled bool) {
state := ctx.State()
if state.MaybeFixUpResourceInstanceAddressForCount(addr, countEnabled) {
log.Printf("[TRACE] renamed first %s instance in transient state due to count argument change", addr)
}
refreshState := ctx.RefreshState()
if refreshState != nil && refreshState.MaybeFixUpResourceInstanceAddressForCount(addr, countEnabled) {
log.Printf("[TRACE] renamed first %s instance in transient state due to count argument change", addr)
}
}

View File

@ -152,11 +152,6 @@ func (b *ApplyGraphBuilder) Steps() []GraphTransformer {
// Target
&TargetsTransformer{Targets: b.Targets},
// Add the node to fix the state count boundaries
&CountBoundaryTransformer{
Config: b.Config,
},
// Close opened plugin connections
&CloseProviderTransformer{},

View File

@ -5,10 +5,12 @@ import (
"strings"
"testing"
"github.com/google/go-cmp/cmp"
"github.com/zclconf/go-cty/cty"
"github.com/hashicorp/terraform/internal/addrs"
"github.com/hashicorp/terraform/internal/plans"
"github.com/hashicorp/terraform/internal/states"
"github.com/zclconf/go-cty/cty"
)
func TestApplyGraphBuilder_impl(t *testing.T) {
@ -60,11 +62,10 @@ func TestApplyGraphBuilder(t *testing.T) {
t.Fatalf("wrong path %q", g.Path.String())
}
actual := strings.TrimSpace(g.String())
expected := strings.TrimSpace(testApplyGraphBuilderStr)
if actual != expected {
t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected)
got := strings.TrimSpace(g.String())
want := strings.TrimSpace(testApplyGraphBuilderStr)
if diff := cmp.Diff(want, got); diff != "" {
t.Fatalf("wrong result\n%s", diff)
}
}
@ -352,10 +353,10 @@ func TestApplyGraphBuilder_destroyCount(t *testing.T) {
t.Fatalf("wrong module path %q", g.Path)
}
actual := strings.TrimSpace(g.String())
expected := strings.TrimSpace(testApplyGraphBuilderDestroyCountStr)
if actual != expected {
t.Fatalf("wrong result\n\ngot:\n%s\n\nwant:\n%s", actual, expected)
got := strings.TrimSpace(g.String())
want := strings.TrimSpace(testApplyGraphBuilderDestroyCountStr)
if diff := cmp.Diff(want, got); diff != "" {
t.Fatalf("wrong result\n%s", diff)
}
}
@ -699,9 +700,6 @@ func TestApplyGraphBuilder_orphanedWithProvider(t *testing.T) {
}
const testApplyGraphBuilderStr = `
meta.count-boundary (EachMode fixup)
module.child (close)
test_object.other
module.child (close)
module.child.test_object.other
module.child (expand)
@ -721,7 +719,7 @@ provider["registry.terraform.io/hashicorp/test"] (close)
module.child.test_object.other
test_object.other
root
meta.count-boundary (EachMode fixup)
module.child (close)
provider["registry.terraform.io/hashicorp/test"] (close)
test_object.create
test_object.create (expand)
@ -735,13 +733,10 @@ test_object.other (expand)
`
const testApplyGraphBuilderDestroyCountStr = `
meta.count-boundary (EachMode fixup)
test_object.B
provider["registry.terraform.io/hashicorp/test"]
provider["registry.terraform.io/hashicorp/test"] (close)
test_object.B
root
meta.count-boundary (EachMode fixup)
provider["registry.terraform.io/hashicorp/test"] (close)
test_object.A (expand)
provider["registry.terraform.io/hashicorp/test"]

View File

@ -156,11 +156,6 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer {
// node due to dependency edges, to avoid graph cycles during apply.
&ForcedCBDTransformer{},
// Add the node to fix the state count boundaries
&CountBoundaryTransformer{
Config: b.Config,
},
// Close opened plugin connections
&CloseProviderTransformer{},

View File

@ -4,10 +4,12 @@ import (
"strings"
"testing"
"github.com/google/go-cmp/cmp"
"github.com/zclconf/go-cty/cty"
"github.com/hashicorp/terraform/internal/addrs"
"github.com/hashicorp/terraform/internal/configs/configschema"
"github.com/hashicorp/terraform/internal/providers"
"github.com/zclconf/go-cty/cty"
)
func TestPlanGraphBuilder_impl(t *testing.T) {
@ -45,10 +47,10 @@ func TestPlanGraphBuilder(t *testing.T) {
t.Fatalf("wrong module path %q", g.Path)
}
actual := strings.TrimSpace(g.String())
expected := strings.TrimSpace(testPlanGraphBuilderStr)
if actual != expected {
t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual)
got := strings.TrimSpace(g.String())
want := strings.TrimSpace(testPlanGraphBuilderStr)
if diff := cmp.Diff(want, got); diff != "" {
t.Fatalf("wrong result\n%s", diff)
}
}
@ -92,15 +94,12 @@ func TestPlanGraphBuilder_dynamicBlock(t *testing.T) {
// is that at the end test_thing.c depends on both test_thing.a and
// test_thing.b. Other details might shift over time as other logic in
// the graph builders changes.
actual := strings.TrimSpace(g.String())
expected := strings.TrimSpace(`
meta.count-boundary (EachMode fixup)
test_thing.c (expand)
got := strings.TrimSpace(g.String())
want := strings.TrimSpace(`
provider["registry.terraform.io/hashicorp/test"]
provider["registry.terraform.io/hashicorp/test"] (close)
test_thing.c (expand)
root
meta.count-boundary (EachMode fixup)
provider["registry.terraform.io/hashicorp/test"] (close)
test_thing.a (expand)
provider["registry.terraform.io/hashicorp/test"]
@ -110,8 +109,8 @@ test_thing.c (expand)
test_thing.a (expand)
test_thing.b (expand)
`)
if actual != expected {
t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual)
if diff := cmp.Diff(want, got); diff != "" {
t.Fatalf("wrong result\n%s", diff)
}
}
@ -150,23 +149,20 @@ func TestPlanGraphBuilder_attrAsBlocks(t *testing.T) {
// list-of-objects attribute. This requires some special effort
// inside lang.ReferencesInBlock to make sure it searches blocks of
// type "nested" along with an attribute named "nested".
actual := strings.TrimSpace(g.String())
expected := strings.TrimSpace(`
meta.count-boundary (EachMode fixup)
test_thing.b (expand)
got := strings.TrimSpace(g.String())
want := strings.TrimSpace(`
provider["registry.terraform.io/hashicorp/test"]
provider["registry.terraform.io/hashicorp/test"] (close)
test_thing.b (expand)
root
meta.count-boundary (EachMode fixup)
provider["registry.terraform.io/hashicorp/test"] (close)
test_thing.a (expand)
provider["registry.terraform.io/hashicorp/test"]
test_thing.b (expand)
test_thing.a (expand)
`)
if actual != expected {
t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual)
if diff := cmp.Diff(want, got); diff != "" {
t.Fatalf("wrong result\n%s", diff)
}
}
@ -211,12 +207,12 @@ func TestPlanGraphBuilder_forEach(t *testing.T) {
t.Fatalf("wrong module path %q", g.Path)
}
actual := strings.TrimSpace(g.String())
got := strings.TrimSpace(g.String())
// We're especially looking for the edge here, where aws_instance.bat
// has a dependency on aws_instance.boo
expected := strings.TrimSpace(testPlanGraphBuilderForEachStr)
if actual != expected {
t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual)
want := strings.TrimSpace(testPlanGraphBuilderForEachStr)
if diff := cmp.Diff(want, got); diff != "" {
t.Fatalf("wrong result\n%s", diff)
}
}
@ -230,9 +226,6 @@ aws_security_group.firewall (expand)
provider["registry.terraform.io/hashicorp/aws"]
local.instance_id (expand)
aws_instance.web (expand)
meta.count-boundary (EachMode fixup)
aws_load_balancer.weblb (expand)
output.instance_id
openstack_floating_ip.random (expand)
provider["registry.terraform.io/hashicorp/openstack"]
output.instance_id
@ -245,7 +238,7 @@ provider["registry.terraform.io/hashicorp/openstack"]
provider["registry.terraform.io/hashicorp/openstack"] (close)
openstack_floating_ip.random (expand)
root
meta.count-boundary (EachMode fixup)
output.instance_id
provider["registry.terraform.io/hashicorp/aws"] (close)
provider["registry.terraform.io/hashicorp/openstack"] (close)
var.foo
@ -263,12 +256,6 @@ aws_instance.boo (expand)
provider["registry.terraform.io/hashicorp/aws"]
aws_instance.foo (expand)
provider["registry.terraform.io/hashicorp/aws"]
meta.count-boundary (EachMode fixup)
aws_instance.bar (expand)
aws_instance.bar2 (expand)
aws_instance.bat (expand)
aws_instance.baz (expand)
aws_instance.foo (expand)
provider["registry.terraform.io/hashicorp/aws"]
provider["registry.terraform.io/hashicorp/aws"] (close)
aws_instance.bar (expand)
@ -277,6 +264,5 @@ provider["registry.terraform.io/hashicorp/aws"] (close)
aws_instance.baz (expand)
aws_instance.foo (expand)
root
meta.count-boundary (EachMode fixup)
provider["registry.terraform.io/hashicorp/aws"] (close)
`

View File

@ -1,80 +0,0 @@
package terraform
import (
"fmt"
"log"
"github.com/hashicorp/terraform/internal/addrs"
"github.com/hashicorp/terraform/internal/configs"
"github.com/hashicorp/terraform/internal/tfdiags"
)
// NodeCountBoundary fixes up any transitions between "each modes" in objects
// saved in state, such as switching from NoEach to EachInt.
type NodeCountBoundary struct {
Config *configs.Config
}
var _ GraphNodeExecutable = (*NodeCountBoundary)(nil)
func (n *NodeCountBoundary) Name() string {
return "meta.count-boundary (EachMode fixup)"
}
// GraphNodeExecutable
func (n *NodeCountBoundary) Execute(ctx EvalContext, op walkOperation) (diags tfdiags.Diagnostics) {
// We'll temporarily lock the state to grab the modules, then work on each
// one separately while taking a lock again for each separate resource.
// This means that if another caller concurrently adds a module here while
// we're working then we won't update it, but that's no worse than the
// concurrent writer blocking for our entire fixup process and _then_
// adding a new module, and in practice the graph node associated with
// this eval depends on everything else in the graph anyway, so there
// should not be concurrent writers.
state := ctx.State().Lock()
moduleAddrs := make([]addrs.ModuleInstance, 0, len(state.Modules))
for _, m := range state.Modules {
moduleAddrs = append(moduleAddrs, m.Addr)
}
ctx.State().Unlock()
for _, addr := range moduleAddrs {
cfg := n.Config.DescendentForInstance(addr)
if cfg == nil {
log.Printf("[WARN] Not fixing up EachModes for %s because it has no config", addr)
continue
}
if err := n.fixModule(ctx, addr); err != nil {
diags = diags.Append(err)
return diags
}
}
return diags
}
func (n *NodeCountBoundary) fixModule(ctx EvalContext, moduleAddr addrs.ModuleInstance) error {
ms := ctx.State().Module(moduleAddr)
cfg := n.Config.DescendentForInstance(moduleAddr)
if ms == nil {
// Theoretically possible for a concurrent writer to delete a module
// while we're running, but in practice the graph node that called us
// depends on everything else in the graph and so there can never
// be a concurrent writer.
return fmt.Errorf("[WARN] no state found for %s while trying to fix up EachModes", moduleAddr)
}
if cfg == nil {
return fmt.Errorf("[WARN] no config found for %s while trying to fix up EachModes", moduleAddr)
}
for _, r := range ms.Resources {
rCfg := cfg.Module.ResourceByAddr(r.Addr.Resource)
if rCfg == nil {
log.Printf("[WARN] Not fixing up EachModes for %s because it has no config", r.Addr)
continue
}
hasCount := rCfg.Count != nil
fixResourceCountSetTransition(ctx, r.Addr.Config(), hasCount)
}
return nil
}

View File

@ -1,72 +0,0 @@
package terraform
import (
"testing"
"github.com/hashicorp/hcl/v2/hcltest"
"github.com/hashicorp/terraform/internal/addrs"
"github.com/hashicorp/terraform/internal/configs"
"github.com/hashicorp/terraform/internal/states"
"github.com/zclconf/go-cty/cty"
)
func TestNodeCountBoundaryExecute(t *testing.T) {
// Create a state with a single instance (addrs.NoKey) of test_instance.foo
state := states.NewState()
state.Module(addrs.RootModuleInstance).SetResourceInstanceCurrent(
addrs.Resource{
Mode: addrs.ManagedResourceMode,
Type: "test_instance",
Name: "foo",
}.Instance(addrs.NoKey),
&states.ResourceInstanceObjectSrc{
Status: states.ObjectReady,
AttrsJSON: []byte(`{"type":"string","value":"hello"}`),
},
addrs.AbsProviderConfig{
Provider: addrs.NewDefaultProvider("test"),
Module: addrs.RootModule,
},
)
// Create a config that uses count to create 2 instances of test_instance.foo
rc := &configs.Resource{
Mode: addrs.ManagedResourceMode,
Type: "test_instance",
Name: "foo",
Count: hcltest.MockExprLiteral(cty.NumberIntVal(2)),
Config: configs.SynthBody("", map[string]cty.Value{
"test_string": cty.StringVal("hello"),
}),
}
config := &configs.Config{
Module: &configs.Module{
ManagedResources: map[string]*configs.Resource{
"test_instance.foo": rc,
},
},
}
ctx := &MockEvalContext{
StateState: state.SyncWrapper(),
}
node := NodeCountBoundary{Config: config}
diags := node.Execute(ctx, walkApply)
if diags.HasErrors() {
t.Fatalf("unexpected error: %s", diags.Err())
}
if !state.HasResources() {
t.Fatal("resources missing from state")
}
// verify that the resource changed from test_instance.foo to
// test_instance.foo.0 in the state
actual := state.String()
expected := "test_instance.foo.0:\n ID = \n provider = provider[\"registry.terraform.io/hashicorp/test\"]\n type = string\n value = hello"
if actual != expected {
t.Fatalf("wrong result: %s", actual)
}
}

View File

@ -304,10 +304,6 @@ func (n *NodePlannableResource) ModifyCreateBeforeDestroy(v bool) error {
func (n *NodePlannableResource) DynamicExpand(ctx EvalContext) (*Graph, error) {
var diags tfdiags.Diagnostics
// We need to potentially rename an instance address in the state
// if we're transitioning whether "count" is set at all.
fixResourceCountSetTransition(ctx, n.Addr.Config(), n.Config.Count != nil)
// Our instance expander should already have been informed about the
// expansion of this resource and of all of its containing modules, so
// it can tell us which instance addresses we need to process.

View File

@ -1,33 +0,0 @@
package terraform
import (
"github.com/hashicorp/terraform/internal/configs"
"github.com/hashicorp/terraform/internal/dag"
)
// CountBoundaryTransformer adds a node that depends on everything else
// so that it runs last in order to clean up the state for nodes that
// are on the "count boundary": "foo.0" when only one exists becomes "foo"
type CountBoundaryTransformer struct {
Config *configs.Config
}
func (t *CountBoundaryTransformer) Transform(g *Graph) error {
node := &NodeCountBoundary{
Config: t.Config,
}
g.Add(node)
// Depends on everything
for _, v := range g.Vertices() {
// Don't connect to ourselves
if v == node {
continue
}
// Connect!
g.Connect(dag.BasicEdge(node, v))
}
return nil
}