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 destroy-time handling of outputs and local values #17241

Merged
merged 9 commits into from
Jan 31, 2018

Commits on Jan 29, 2018

  1. always evaluate locals, even during destroy

    Destroy-time provisioners require us to re-evaluate during destroy.
    
    Rather than destroying local values, which doesn't do much since they
    aren't persisted to state, we always evaluate them regardless of the
    type of apply. Since the destroy-time local node is no longer a
    "destroy" operation, the order of evaluation need to be reversed. Take
    the existing DestroyValueReferenceTransformer and change it to reverse
    the outgoing edges, rather than in incoming edges. This makes it so that
    any dependencies of a local or output node are destroyed after
    evaluation.
    
    Having locals evaluated during destroy failed one other test, but that
    was the odd case where we need `id` to exist as an attribute as well as
    a field.
    jbardin committed Jan 29, 2018
    Configuration menu
    Copy the full SHA
    7da1a39 View commit details
    Browse the repository at this point in the history
  2. add destroy provisioner test with locals, outputs

    Add a complex destroy provisioner testcase using locals, outputs and
    variables.
    
    Add that pesky "id" attribute to the instance states for interpolation.
    jbardin committed Jan 29, 2018
    Configuration menu
    Copy the full SHA
    7ac0a46 View commit details
    Browse the repository at this point in the history
  3. always evaluate outputs too

    Always evaluate outputs during destroy, just like we did for locals.
    This breaks existing tests, which we will handle separately.
    
    Don't reverse output/local node evaluation order during destroy, as they
    are both being evaluated.
    jbardin committed Jan 29, 2018
    Configuration menu
    Copy the full SHA
    0813955 View commit details
    Browse the repository at this point in the history

Commits on Jan 30, 2018

  1. delete outputs during destroy

    Now that outputs are always evaluated, we still need a way to remove
    them from state when they are destroyed.
    
    Previously, outputs were removed during destroy from the same
    "Applyable" node type that evaluates them. Now that we need to possibly
    both evaluate and remove output during an apply, we add a new node -
    NodeDestroyableOutput.
    
    This new node is added to the graph by the DestroyOutputTransformer,
    which make the new destroy node depend on all descendants of the output
    node.  This ensures that the output remains in the state as long as
    everything which may interpolate the output still exists.
    jbardin committed Jan 30, 2018
    Configuration menu
    Copy the full SHA
    d31fe5a View commit details
    Browse the repository at this point in the history
  2. add a more complex locals test

    Using destroy provisioners again for edge cases during destroy.
    jbardin committed Jan 30, 2018
    Configuration menu
    Copy the full SHA
    2d138d9 View commit details
    Browse the repository at this point in the history
  3. add PruneUnusedValuesTransformer

    Since outputs and local nodes are always evaluated, if the reference a
    resource form the configuration that isn't in the state, the
    interpolation could fail.
    
    Prune any local or output values that have no references in the graph.
    jbardin committed Jan 30, 2018
    Configuration menu
    Copy the full SHA
    99867f0 View commit details
    Browse the repository at this point in the history
  4. catch missing id attribute during interpolation

    The id attribute can be missing during the destroy operation.
    While the new destroy-time ordering of outputs and locals should prevent
    resources from having their id attributes set to an empty string,
    there's no reason to error out if we have the canonical ID field
    available.
    
    This still interrogates the attributes map first to retain any previous
    behavior, but in the future we should settle on a single ID location.
    jbardin committed Jan 30, 2018
    Configuration menu
    Copy the full SHA
    a2f8482 View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    ca4178b View commit details
    Browse the repository at this point in the history

Commits on Jan 31, 2018

  1. Make sure outputs are removed when targeting

    Similar to NodeApplyableOuptut, NodeDestroyableOutputs also need to stay
    in the graph if any ancestor nodes
    
    Use the same GraphNodeTargetDownstream method to keep them from being
    pruned, since they are dependent on the output node and all its
    descendants.
    jbardin committed Jan 31, 2018
    Configuration menu
    Copy the full SHA
    7fbc35a View commit details
    Browse the repository at this point in the history