774 Commits (6dcf00aefc56b48bae54822a6e62eacffd2bc3ea)

Author	SHA1	Message	Date
Krista LaFentres (she/her)	6dcf00aefc	Merge pull request #30344 from hashicorp/lafentres/refactor-show-command ... cli: Refactor show command & migrate to command arguments and views	4 years ago
Krista LaFentres	64e1241ae3	backend/local: Remove unused DisablePlanFileStateLineageChecks flag ... Now that show command has been refactored to remove its dependence on a local backend and local run, this flag is no longer needed to fix #30195.	4 years ago
Krista LaFentres	fea8f6cfa2	cli: Migrate show command to use command arguments and views	4 years ago
Krista LaFentres	8d1bced812	cli: Refactor show command to remove dependence on local run and only load the backend when we need it ... See https://github.com/hashicorp/terraform/pull/30205#issuecomment-997113175 for more context	4 years ago
Martin Atkins	e95f29bf9d	lang/funcs: fileexists slightly better "not a file" error message ... Previously we were just returning a string representation of the file mode, which spends more characters on the irrelevant permission bits that it does on the directory entry type, and is presented in a Unix-centric format that likely won't be familiar to the user of a Windows system. Instead, we'll recognize a few specific directory entry types that seem worth mentioning by name, and then use a generic message for the rest. The original motivation here was actually to deal with the fact that our tests for this function were previously not portable due to the error message leaking system-specific permission detail that are not relevant to the test. Rather than just directly addressing that portability problem, I took the opportunity to improve the error messages at the same time. However, because of that initial focus there are only actually tests here for the directory case. A test that tries to test any of these other file modes would not be portable and in some cases would require superuser access, so we'll just leave those cases untested for the moment since they weren't tested before anyway, and so we've not _lost_ any test coverage here.	4 years ago
Martin Atkins	bdc5f152d7	refactoring: Implied move statements can be cross-package ... Terraform uses "implied" move statements to represent the situation where it automatically handles a switch from count to no-count on a resource. Because that situation requires targeting only a specific resource instance inside a specific module instance, implied move statements are always presented as if they had been declared in the root module and then traversed through the exact module instance path to reach the target resource. However, that means they can potentially cross a module package boundary, if the changed resource belongs to an external module. Normally we prohibit that to avoid the root module depending on implementation details of the called module, but Terraform generates these implied statements based only on information in the called module and so there's no need to apply that same restriction to implied move statements, which will always have source and destination addresses belonging to the same module instance. This change therefore fixes a misbehavior where Terraform would reject an attempt to switch from no-count to count in a called module, where previously the author of the calling configuration had no recourse to fix it because the change has actually happened upstream.	4 years ago
James Bardin	684ed7505d	remove synthetic default expression for variables ... Now that variable evaluation checks for a nil expression the graph transformer does not need to generate a synthetic expression for variable defaults. This means that all default handling is now located in one place, and we are not surprised by a configuration expression showing up which doesn't actually exist in the configuration. Rename nodeModuleVariable.evalModuleCallArgument to evalModuleVariable. This method is no longer handling only the module call argument, it is also dealing with the variable declaration defaults and validation statements. Add an additional tests for validation with a non-nullable variable.	4 years ago
Martin Atkins	9ebc3e1cd2	core: More accurate error message for invalid variable values ... In earlier Terraform versions we had an extra validation step prior to the graph walk which tried to partially validate root module input variable values (just checking their type constraints) and then return error messages which specified as accurately as possible where the value had originally come from. We're now handling that sort of validation exclusively during the graph walk so that we can share the main logic between both root module and child module variable values, but previously that shared code wasn't able to generate such specific information about where the values had originated, because it was adapted from code originally written to only deal with child module variables. Here then we restore a similar level of detail as before, when we're processing root module variables. For child module variables, we use synthetic InputValue objects which state that the value was declared in the configuration, thus causing us to produce a similar sort of error message as we would've before which includes a source range covering the argument expression in the calling module block.	4 years ago
Martin Atkins	36c4d4c241	core and backend: remove redundant handling of default variable values ... Previously we had three different layers all thinking they were responsible for substituting a default value for an unset root module variable: - the local backend, via logic in backend.ParseVariableValues - the context.Plan function (and other similar functions) trying to preprocess the input variables using terraform.mergeDefaultInputVariableValues . - the newer prepareFinalInputVariableValue, which aims to centralize all of the variable preparation logic so it can be common to both root and child module variables. The second of these was also trying to handle type constraint checking, which is also the responsibility of the central function and not something we need to handle so early. Only the last of these consistently handles both root and child module variables, and so is the one we ought to keep. The others are now redundant and are causing prepareFinalInputVariableValue to get a slightly corrupted view of the caller's chosen variable values. To rectify that, here we remove the two redundant layers altogether and have unset root variables pass through as cty.NilVal all the way to the central prepareFinalInputVariableValue function, which will then handle them in a suitable way which properly respects the "nullable" setting. This commit includes some test changes in the terraform package to make those tests no longer rely on the mergeDefaultInputVariableValues logic we've removed, and to instead explicitly set cty.NilVal for all unset variables to comply with our intended contract for PlanOpts.SetVariables, and similar. (This is so that we can more easily catch bugs in callers where they _don't_ correctly handle input variables; it allows us to distinguish between the caller explicitly marking a variable as unset vs. not describing it at all, where the latter is a bug in the caller.)	4 years ago
Martin Atkins	37b1413ab3	core: Handle root and child module input variables consistently ... Previously we had a significant discrepancy between these two situations: we wrote the raw root module variables directly into the EvalContext and then applied type conversions only at expression evaluation time, while for child modules we converted and validated the values while visiting the variable graph node and wrote only the _final_ value into the EvalContext. This confusion seems to have been the root cause for #29899, where validation rules for root module variables were being applied at the wrong point in the process, prior to type conversion. To fix that bug and also make similar mistakes less likely in the future, I've made the root module variable handling more like the child module variable handling in the following ways: - The "raw value" (exactly as given by the user) lives only in the graph node representing the variable, which mirrors how the _expression_ for a child module variable lives in its graph node. This means that the flow for the two is the same except that there's no expression evaluation step for root module variables, because they arrive as constant values from the caller. - The set of variable values in the EvalContext is always only "final" values, after type conversion is complete. That in turn means we no longer need to do "just in time" conversion in evaluationStateData.GetInputVariable, and can just return the value exactly as stored, which is consistent with how we handle all other references between objects. This diff is noisier than I'd like because of how much it takes to wire a new argument (the raw variable values) through to the plan graph builder, but those changes are pretty mechanical and the interesting logic lives inside the plan graph builder itself, in NodeRootVariable, and the shared helper functions in eval_variable.go. While here I also took the opportunity to fix a historical API wart in EvalContext, where SetModuleCallArguments was built to take a set of variable values all at once but our current caller always calls with only one at a time. That is now just SetModuleCallArgument singular, to match with the new SetRootModuleArgument to deal with root module variables.	4 years ago
Martin Atkins	483c38aca1	core: Remove TestContext2Validate_PlanGraphBuilder ... This test seems to be a holdover from the many-moons-ago switch from one graph for all operations to separate graphs for plan and apply. It is effectively just a copy of a subset of the content of the Context.Validate function and is a maintainability hazard because it tends to lag behind updates to that function unless changes there happen to make it fail. This test doesn't cover anything that the other validate context tests don't exercise as an implementation detail of calling Context.Validate, so I've just removed it with no replacement.	4 years ago
Martin Atkins	171e7ef6d9	core: Invalid for_each argument messaging improvements ... Our original messaging here was largely just lifted from the equivalent message for unknown values in "count", and it didn't really include any specific advice on how to update a configuration to make for_each valid, instead focusing only on the workaround of using the -target planning option. It's tough to pack in a fully-actionable suggestion here since unknown values in for_each keys tends to be a gnarly architectural problem rather than a local quirk -- when data flows between modules it can sometimes be unclear whether it'll end up being used in a context which allows unknown values. I did my best to summarize the advice we've been giving in community forum though, in the hope that more people will be able to address this for themselves without asking for help, until we're one day able to smooth this out better with a mechanism such as "partial apply".	4 years ago
Nick Fagerlund	1e9075b4fa	Merge pull request #30226 from hashicorp/nf/dec21-derandomize-dependencies ... Sort dependencies when encoding `ResourceInstanceObject`	4 years ago
Nick Fagerlund	05d0febf7f	Relax test to focus on the behavior we care about (encoded == encoded) ... The specific output order is meaningless, but it should always be the same after two Encode() calls with identical (ignoring in-memory order) dependency sets.	4 years ago
Martin Atkins	b802db75d7	build: Build and run e2etest as part of the release build pipeline ... This uses the decoupled build and run strategy to run the e2etests so that we can arrange to run the tests against the real release packages produced elsewhere in this workflow, rather than ones generated just in time by the test harness. The modifications to make-archive.sh here make it more consistent with its originally-intended purpose of producing a harness for testing "real" release executables. Our earlier compromise of making it include its own terraform executable came from a desire to use that script as part of manual cross-platform testing when we weren't yet set up to support automation of those tests as we're doing here. That does mean, however, that the terraform-e2etest package content must be combined with content from a terraform release package in order to produce a valid contest for running the tests. We use a single job to cross-compile the test harness for all of the supported platforms, because that build is relatively fast and so not worth the overhead of matrix build, but then use a matrix build to actually run the tests so that we can run them in a worker matching the target platform. We currently have access only to amd64 (x64) runners in GitHub Actions and so for the moment this process is limited only to the subset of our supported platforms which use that architecture.	4 years ago
Alisdair McDiarmid	df36a03be1	states: Add failing test for ordered dependencies	4 years ago
Alisdair McDiarmid	535da4ebc7	Merge pull request #30205 from hashicorp/alisdair/fix-show-plan-against-non-default-state ... command/show: Disable plan state lineage checks	4 years ago
Katy Moe	f8fdb6de3f	do not use pointer addr strings as map keys in set ... When creating a Set of BasicEdges, the Hashcode function is used to determine map keys for the underlying set data structure. The string hex representation of the two vertices' pointers is unsafe to use as a map key, since these addresses may change between the time they are added to the set and the time the set is operated on. Instead we modify the Hashcode function to maintain the references to the underlying vertices so they cannot be garbage collected during the lifetime of the Set.	4 years ago
James Bardin	9272ff2c29	Merge pull request #30286 from hashicorp/jbardin/dag ... dag: minor cleanup	4 years ago
James Bardin	8bbba22f8c	Merge pull request #30253 from hashicorp/jbardin/move-graph ... cleanup some move graph handling	4 years ago
Alisdair McDiarmid	ef01d5d134	Merge pull request #30067 from hashicorp/alisdair/redact-sensitive-values-from-function-errors ... lang/funcs: Redact sensitive values from function errors	4 years ago
James Bardin	344adb6c50	clarify dag comments ... TransitiveReduction does not rely on having a single root, and only must be free of cycles. DepthFirstWalk and ReverseDepthFirstWalk do not do a topological sort, so if order matters TransitiveReduction must be run first.	4 years ago
James Bardin	fae68f166f	Remove sorted walk functions ... These two functions were left during a refactor to ensure the old behavior of a sorted walk was still accessible in some manner. The package has since been removed from any public API, and the sorted versions are no longer called, so we can remove them.	4 years ago
James Bardin	f46cf7b8bc	cleanup some move graph handling ... Create a separate `validateMoveStatementGraph` function so that `ValidateMoves` and `ApplyMoves` both check the same conditions. Since we're not using the builtin `graph.Validate` method, because we may have multiple roots and want better cycle diagnostics, we need to add checks for self references too. While multiple roots are an error enforced by `Validate` for the concurrent walk, they are OK when using `TransitiveReduction` and `ReverseDepthFirstWalk`, so we can skip that check. Apply moves must first use `TransitiveReduction` to reduce the graph, otherwise nodes may be skipped if they are passed over by a transitive edge.	4 years ago
James Bardin	22dc685052	check for nested module index changes ... Changing only the index on a nested module will cause all nested moves to create cycles, since their full addresses will match both the From and To addresses. When building the dependency graph, check if the parent is only changing the index of the containing module, and prevent the backwards edge for the move.	4 years ago
James Bardin	deb82daf2b	find implied moves in nested modules ... Implied moves in nested modules were being skipped	4 years ago
James Bardin	3d769b7282	IsModuleMoveReIndex ... Add a method for checking if the From and To addresses in a move statement are only changing the indexes of modules relative to the statement module. This is needed because move statement nested within the module will be able to match against both the From and To addresses, causing cycles in the order of move operations.	4 years ago
Martin Atkins	74761b2f8b	getmodules: Use go-getter v1.5.10 and return to upstream GitGetter ... There was an unintended regression in go-getter v1.5.9's GitGetter which caused us to temporarily fork that particular getter into Terraform to expedite a fix. However, upstream v1.5.10 now includes a functionally-equivalent fix and so we can heal that fork by upgrading. We'd also neglected to update the Module Sources docs when upgrading to go-getter v1.5.9 originally and so we were missing documentation about the new "depth" argument to enable shadow cloning, which I've added retroactively here along with documenting its restriction of only supporting named refs. This new go-getter release also introduces a new credentials-passing method for the Google Cloud Storage getter, and so we must incorporate that into the Terraform-level documentation about module sources.	4 years ago
James Bardin	66b4d155b1	Merge pull request #30232 from hashicorp/jbardin/module-move-re-index ... Handle move blocks within a module which is changing the index	4 years ago
James Bardin	75ef61c783	check for nested module index changes ... Changing only the index on a nested module will cause all nested moves to create cycles, since their full addresses will match both the From and To addresses. When building the dependency graph, check if the parent is only changing the index of the containing module, and prevent the backwards edge for the move.	4 years ago
Barrett Clark	296acdd961	Merge pull request #30020 from hashicorp/barrettclark/cloud-e2e-parallel ... Cloud: Add parallelism back into the tests	4 years ago
Alisdair McDiarmid	f772cb085e	Merge pull request #30233 from hashicorp/alisdair/move-nested-modules ... refactoring: Move nested modules	4 years ago
James Bardin	a72d2d408d	Merge pull request #30095 from hashicorp/jbardin/invalid-provider-name ... skip provider resolution when there are errors	4 years ago
James Bardin	e761117562	find implied moves in nested modules ... Implied moves in nested modules were being skipped	4 years ago
James Bardin	346418e31f	IsModuleMoveReIndex ... Add a method for checking if the From and To addresses in a move statement are only changing the indexes of modules relative to the statement module. This is needed because move statement nested within the module will be able to match against both the From and To addresses, causing cycles in the order of move operations.	4 years ago
Alisdair McDiarmid	d7ef123c12	refactoring: Move nested modules ... When applying module `moved` statements by iterating through modules in state, we previously required an exact match from the `moved` statement's `from` field and the module address. This permitted moving resources directly inside a module, but did not recur into module calls within those moved modules. This commit moves that exact match requirement so that it only applies to `moved` statements targeting resources. In turn this allows nested modules to be moved.	4 years ago
Nick Fagerlund	9b449bec99	Sort dependencies when encoding `ResourceInstanceObject` ... Resource dependencies are by nature an unordered collection, but they're persisted to state as a JSON array (in random order). This makes a mess for `terraform apply -refresh-only`, which sees the new random order as a change that requires the user to approve a state update. (As an additional problem on top of that, the user interface for refresh-only runs doesn't expect to see that as a type of change, so it says "no changes! would you like to update to reflect these detected changes?") This commit changes `ResourceInstanceObject.Encode()` to sort the in-memory slice of dependencies (lexically, by address) before passing it on to be compared and persisted. This appears to fix the observed UI issues with a minimum of logic changes.	4 years ago
Barrett Clark	d196d2870a	Refactor cloud table test runs ... As the cloud e2e tests evolved some common patters became apparent. This standardizes and consolidates the patterns into a common test runner that takes the table tests and runs them in parallel. Some tests also needed to be converted to utilize table tests.	4 years ago
Martin Atkins	23395a1022	providercache: Discard lock entries for unused providers ... Previously we would only ever add new lock entries or update existing ones. However, it's possible that over time a module may _cease_ using a particular provider, at which point we ought to remove it from the lock file so that operations won't fail when seeing that the provider cache directory is inconsistent with the lock file. Now the provider installer (EnsureProviderVersions) will remove any lock file entries that relate to providers not included in the given requirements, which therefore makes the resulting lock file properly match the set of packages the installer wrote into the cache. This does potentially mean that someone could inadvertently defeat the lock by removing a provider dependency, running "terraform init", then undoing that removal, and finally running "terraform init" again. However, that seems relatively unlikely compared to the likelihood of removing a provider and keeping it removed, and in the event it _did_ happen the changes to the lock entry for that provider would be visible in the diff of the provider lock file as usual, and so could be noticed in code review just as for any other change to dependencies.	4 years ago
Alisdair McDiarmid	768741c0f7	command/show: Disable plan state lineage checks ... When showing a saved plan, we do not need to check the state lineage against current state, because the plan cannot be applied. This is relevant when plan and apply specify a `-state` argument to choose a non-default state file. In this case, the stored prior state in the plan will not match the default state file, so a lineage check will always error.	4 years ago
James Bardin	8c4031ef15	don't persist a nil state from Apply ... Apply should not return a nil state to be persisted.	4 years ago
James Bardin	58e001c22a	return graph errors from Context.Apply ... errors from building during apply were lost	4 years ago
James Bardin	f83ed441bb	Merge pull request #30189 from hashicorp/jbardin/validate-moves ... Ignore unexpanded paths when validating move statements.	4 years ago
James Bardin	b213386a73	InstancesForModule should not panic ... instances.Set is only used after all instances have been processes, so it should therefor only handle known instances and not panic when given an address that traverses an unexpanded module.	4 years ago
James Bardin	371660ab8f	cleanup panic output	4 years ago
James Bardin	a5017bff2f	failing test moved with target	4 years ago
Barrett Clark	c647b41d65	Add parallelism back into the tests ... Running tests in parallel can help speed up overall test execution. Go blocks parent tests while child tests run, so it does not fully fan out as you might expect. It is noticably faster, though. Running 4 or more concurrent processes knocks over a minute off the total execution time.	4 years ago
James Bardin	e22ab70e03	Merge pull request #30171 from hashicorp/jbardin/revert-validate-for-each ... use `cty.DynamicVal` for expanded resources during validation	4 years ago
James Bardin	645fcc5f12	test for lookup regression during validation	4 years ago
James Bardin	71b9682e8c	ensure there is always a valid return value	4 years ago