18 Commits (b43daeaa8d72d66f5a990ed2ffbe5942e12bcde3)

Author	SHA1	Message	Date
James Bardin	cac1f5c264	refactoring: exhaustive NestedWithin checks ... When checking dependencies between statements, we need to check all combinations of To and From addresses.	5 years ago
Martin Atkins	7b99861b1c	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.	5 years ago
Martin Atkins	83f0376673	refactoring: ApplyMoves new return type ... When we originally stubbed ApplyMoves we didn't know yet how exactly we'd be using the result, so we made it a double-indexed map allowing looking up moves in both directions. However, in practice we only actually need to look up old addresses by new addresses, and so this commit first removes the double indexing so that each move is only represented by one element in the map. We also need to describe situations where a move was blocked, because in a future commit we'll generate some warnings in those cases. Therefore ApplyMoves now returns a MoveResults object which contains both a map of changes and a map of blocks. The map of blocks isn't used yet as of this commit, but we'll use it in a later commit to produce warnings within the "terraform" package.	5 years ago
Martin Atkins	ee9e346039	refactoring: ApplyMoves skips moving when destination address occupied ... Per our rule that the content of the state can never make a move statement invalid, our behavior for two objects trying to occupy the same address will be to just ignore that and let the object already at the address take priority. For the moment this is silent from an end-user perspective and appears only in our internal logs. However, I'm hoping that our future planned adjustment to the interface of this function will include some way to allow reporting these collisions in some end-user-visible way, either as a separate warning per collision or as a single warning that collects together all of the collisions into a single message somehow. This situation can arise both because the previous run state already contained an object at the target address of a move and because more than one move ends up trying to target the same location. In the latter case, which one "wins" is decided by our depth-first traversal order, which is in turn derived from our chaining and nesting rules and is therefore arbitrary but deterministic.	5 years ago
Martin Atkins	ef5a1c9cfe	refactoring: ImpliedMoveStatements function ... This new function complements the existing function FindMoveStatements by potentially generating additional "implied" move statements that aren't written explicit in the configuration but that we'll infer by comparing the configuration and te previous run state. The goal here is to infer only enough to replicate the effect of the "count boundary fixup" graph node (terraform.NodeCountBoundary) that we currently use to deal with this concern of preserving the zero-instance when switching between "count" and not "count". This is just dead code for now. A subsequent commit will introduce this into the "terraform" package while also removing terraform.NodeCountBoundary, thus achieving the same effect as before but in a way that'll get reported in the UI as a move, using the same language that we'd use for an explicit move statement.	5 years ago
Martin Atkins	b4594551f7	refactoring: TestValidateMoves/cyclic_chain can now pass ... When originally filling out these test cases we didn't yet have the logic in place to detect chained moves and so this test couldn't succeed in spite of being correct. We now have chain-detection implemented and so consequently we can also detect cyclic chains. This commit largely just enables the original test unchanged, although it does include the text of the final error message for reporting cyclic move chains which wasn't yet finalized when we were stubbing out this test case originally.	5 years ago
Martin Atkins	4faac6ee43	core: Record move result information in the plan ... Here we wire through the "move results" into the graph walk data structures so that all of the the nodes which produce plans.ResourceInstanceChange values can capture the "PrevRunAddr" for each resource instance. This doesn't actually quite work yet, because the logic in Context.Plan isn't actually correct and so the updated state from refactoring.ApplyMoves isn't actually visible as the "previous run state". For that reason, the context test in this commit is currently skipped, with the intent of re-enabling it once the updated state is properly propagating into the plan graph walk and thus we can actually react to the result of the move while choosing actions for those addresses.	5 years ago
James Bardin	30afb492ab	fix ApplyMove test with nested modules working ... The addrs package can now correctly handle the combinations of nested module endpoints, which fixes these 2 tests.	5 years ago
James Bardin	553d6525d2	more move tests	5 years ago
James Bardin	493ec4e6c5	correct the direction and walk order of the graph	5 years ago
James Bardin	08edb02270	MoveStatement.Name() ... makes the graph printable for debugging	5 years ago
James Bardin	789317dc05	additional test	5 years ago
James Bardin	6401022bc8	don't take the address of a range variable	5 years ago
James Bardin	09ab952683	fix ApplyMoves tests ... Add empty result value, since ApplyMoves does not return nil. Fix the desired addresses for moves.	5 years ago
Martin Atkins	aa414f3ab3	refactoring: First round of ValidateMoves rules ... This is a first pass at implementing refactoring.ValidateMoves, covering the main validation rules. This is not yet complete. A couple situations not yet covered are represented by commented test cases in TestValidateMoves, although that isn't necessarily comprehensive. We'll do a further pass of filling this out with any other subtleties before we ship this feature.	5 years ago
Martin Atkins	f76a3467dc	core: Call into refactoring.ValidateMoves after creating a plan ... As of this commit, refactoring.ValidateMoves doesn't actually do anything yet (always returns nil) but the goal here is to wire in the set of all declared instances so that refactoring.ValidateMoves will then have all of the information it needs to encapsulate our validation rules. The actual implementation of refactoring.ValidateMoves will follow in subsequent commits.	5 years ago
Martin Atkins	3e5bfa7364	refactoring: Stubbing of the logic for handling moves ... This is a whole lot of nothing right now, just stubbing out some control flow that ultimately just leads to TODOs that cause it to do nothing at all. My intent here is to get this cross-cutting skeleton in place and thus make it easier for us to collaborate on adding the meat to it, so that it's more likely we can work on different parts separately and still get a result that tessellates.	5 years ago
Martin Atkins	22eee529e3	addrs: MoveEndpointInModule ... We previously built out addrs.UnifyMoveEndpoints with a different implementation strategy in mind, but that design turns out to not be viable because it forces us to move to AbsMoveable addresses too soon, before we've done the analysis required to identify chained and nested moves. Instead, UnifyMoveEndpoints will return a new type MoveEndpointInModule which conceptually represents a matching pattern which either matches or doesn't match a particular AbsMoveable. It does this by just binding the unified relative address from the MoveEndpoint to the module where it was declared, and thus allows us to distinguish between the part of the module path which applies to any instances of the given modules vs. the user-specified part which must identify particular module instances.	5 years ago