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

Extra feedback for a variety of "move" consequences and edge-cases #29637

Merged
merged 5 commits into from
Sep 23, 2021

Commits on Sep 22, 2021

  1. refactoring: Don't implicitly move for resources with for_each

    Our previous rule for implicitly moving from IntKey(0) to NoKey would
    apply that move even when the current resource configuration uses
    for_each, because we were only considering whether "count" were set.
    
    Previously this was relatively harmless because the resource instance in
    question would end up planned for deletion anyway: neither an IntKey nor
    a NoKey are valid keys for for_each.
    
    Now that we're going to be announcing these moves explicitly in the UI,
    it would be confusing to see Terraform report that IntKey moved to NoKey
    in a situation where the config changed from count to for_each, so to
    address that we'll only generate the implied statement if neither
    repetition argument is set.
    apparentlymart committed Sep 22, 2021
    Configuration menu
    Copy the full SHA
    56ab5d8 View commit details
    Browse the repository at this point in the history

Commits on Sep 23, 2021

  1. core: Report ActionReasons when we plan to delete "orphans"

    There are a few different reasons why a resource instance tracked in the
    prior state might be considered an "orphan", but previously we reported
    them all identically in the planned changes.
    
    In order to help users understand the reason for a surprising planned
    delete, we'll now try to specify an additional reason for the planned
    deletion, covering all of the main reasons why that could happen.
    
    This commit only introduces the new detail to the plans.Changes result,
    though it also incidentally exposes it as part of the JSON plan result
    in order to keep that working without returning errors in these new
    cases. We'll expose this information in the human-oriented UI output in
    a subsequent commit.
    apparentlymart committed Sep 23, 2021
    Configuration menu
    Copy the full SHA
    22412de View commit details
    Browse the repository at this point in the history
  2. command/format: Include deletion reasons in plan report

    The core runtime is now able to specify a reason for some situations when
    Terraform plans to delete a resource instance.
    
    This commit makes that information visible in the human-oriented UI. A
    previous commit already made the underlying data informing these new hints
    visible as part of the machine-oriented (JSON) plan output.
    
    This also removes the bold formatting from the existing "has moved to"
    hints, because subjectively it seemed like the result was emphasizing too
    many parts of the output and thus somewhat defeating the benefit of the
    emphasis in trying to create additional visual hierarchy for sighted users
    running Terraform in a terminal. Now only the first line containing the
    main action statement will be in bold, and all of the parenthesized
    follow-up notes will be unformatted.
    apparentlymart committed Sep 23, 2021
    Configuration menu
    Copy the full SHA
    c563586 View commit details
    Browse the repository at this point in the history
  3. core: Report a warning if any moves get blocked

    In most cases Terraform will be able to automatically fully resolve all
    of the pending move statements before creating a plan, but there are some
    edge cases where we can end up wanting to move one object to a location
    where another object is already declared.
    
    One relatively-obvious example is if someone uses "terraform state mv" in
    order to create a set of resource instance bindings that could never have
    arising in normal Terraform use.
    
    A less obvious example arises from the interactions between moves at
    different levels of granularity. If we are both moving a module to a new
    address and moving a resource into an instance of the new module at the
    same time, the old module might well have already had a resource of the
    same name and so the resource move will be unresolvable.
    
    In these situations Terraform will move the objects as far as possible,
    but because it's never valid for a move "from" address to still be
    declared in the configuration Terraform will inevitably always plan to
    destroy the objects that didn't find a final home. To give some additional
    explanation for that result, here we'll add a warning which describes
    what happened.
    
    This is not a particularly actionable warning because we don't really
    have enough information to guess what the user intended, but we do at
    least prompt that they might be able to use the "terraform state" family
    of subcommands to repair the ambiguous situation before planning, if they
    want a different result than what Terraform proposed.
    apparentlymart committed Sep 23, 2021
    Configuration menu
    Copy the full SHA
    b417aa8 View commit details
    Browse the repository at this point in the history
  4. core: Don't return other errors if move statements are invalid

    Because our validation rules depend on some dynamic results produced by
    actually running the plan, we deal with moves in a "backwards" order where
    we try to apply them first -- ignoring anything strange we might find --
    and then validate the original statements only after planning.
    
    An unfortunate consequence of that approach is that when the move
    statements are invalid it's likely that move execution will not fully
    complete, and so the generated plan is likely to be incorrect and might
    well include errors resulting from the unresolved moves.
    
    To mitigate that, here we let any move validation errors supersede all
    other diagnostics that the plan phase might've generated, in the hope that
    it'll help the user focus on fixing the incorrect move statements without
    creating confusing by reporting errors that only appeared as a quick of
    how Terraform worked around the invalid move statements earlier.
    apparentlymart committed Sep 23, 2021
    Configuration menu
    Copy the full SHA
    f371f46 View commit details
    Browse the repository at this point in the history