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

path.root is . instead of full path since Terraform 0.12 #21400

Closed
AndiDog opened this issue May 23, 2019 · 7 comments
Closed

path.root is . instead of full path since Terraform 0.12 #21400

AndiDog opened this issue May 23, 2019 · 7 comments

Comments

@AndiDog
Copy link
Contributor

AndiDog commented May 23, 2019

Terraform Version

Terraform v0.12.0
+ provider.aws v2.11.0

(installed via homebrew by editing source URL to https://github.com/hashicorp/terraform/archive/v0.12.0.tar.gz)

Terraform Configuration Files

resource "aws_instance" "host" {
  # ...

  tags = {
    Foobar                   = "${path.root}"
    CreatedFromTerraformRoot = "${replace(path.root, "/^.*?(my-git-repo-dir\\/)/", "$1")}"
  }
}

Expected Behavior

In v0.11.3, Terraform evaluates ${path.root} to the full path, e.g. /Users/asommer/dev/my-git-repo-dir.

In the above example, I would have expected to get a nice, descriptive EC2 instance tag which tells others from which Terraform files the resource was created, e.g. CreatedFromTerraformRoot=my-git-repo-dir/staging/terraform.

Actual Behavior

With v0.12, ${path.root} evaluates to .

Contrarily, I get this from the console mode:

$ pwd
/Users/asommer/dev/my-git-repo-dir/staging/terraform
$ terraform console
> path.root
/Users/asommer/dev/my-git-repo-dir/staging/terraform

Steps to Reproduce

Put the variable somewhere and look at the plan to see what value gets substituted.

@AndiDog
Copy link
Contributor Author

AndiDog commented May 23, 2019

This is the problem (command/meta_config.go):

func (m *Meta) loadSingleModule(dir string) (*configs.Module, tfdiags.Diagnostics) {
	var diags tfdiags.Diagnostics
	dir = m.normalizePath(dir) // <=================

	loader, err := m.initConfigLoader()
	if err != nil {
		diags = diags.Append(err)
		return nil, diags
	}

	module, hclDiags := loader.Parser().LoadConfigDir(dir)
	diags = diags.Append(hclDiags)
	return module, diags
}

ebafa51#diff-f6b857536a7e1e29fac61431e919a8feL125 has introduced use of this function, I believe, and previously the path wasn't normalized. Since there is no good reason for normalizing the path, I'll put up a PR to go back to using os.Getwd() unchanged.

@AndiDog
Copy link
Contributor Author

AndiDog commented May 23, 2019

Actually, 2cf63d0 clearly says that you broke compatibility on purpose and now always normalize paths. Now what? Revert, keep, finally add an abspath function as proposed in #16697?

@apparentlymart
Copy link
Contributor

Hi @AndiDog,

Yes, this was intentional because encoding details about the machine where Terraform ran into a shared state is undesirable for many users, who want to be able to move their Terraform runs between contexts over time without upsetting things. Now people can use path.module and/or path.root in situations like aws_s3_bucket_object without seeing unnecessary updates when Terraform is run from a different directory. The goal of normalization, then, is to help write modules whose behavior doesn't change when applied on a different machine.

It sounds like in your case you are trying to generate relative paths against a different root... the root of your whole monorepo, I think? The path.cwd value is still absolute to allow for some use-cases like these, but it sounds like in your case you'd benefit most from the pathrel function discussed in #16697 to derive the path relative to that other prefix:

CreatedFromTerraformRoot = pathrel("${path.cwd}/${path.root}"), "/path/to/git-repo")

...though I notice in your example you were using a regex pattern which I assume was intended to avoid the need to actually specify an absolute path by instead relying on the directory containing the git work tree to have a particular name. In that case, pathrel alone would be insufficient, and to do this any better way than regexp-matching the path would require some other sort of function that I'm not sure about: "find nearest directory named x?", "find nearest directory containing a directory called .git"?

It does indeed seem like the missing part here is some functions to manipulate paths in a way that actually treats them as paths, rather than as generic strings... the "${path.cwd}/${path.root}" in my above example would produce /Users/asommer/dev/my-git-repo-dir/something/. using just naive string manipulation, which would work okay for accessing files but does not produce a suitable result when the goal is to capture the filename itself, because it needs a cleaning step applied to notice that the ./ on the end is redundant.

With that said, since path.root evaluated to just . for you, that suggests that in your case path.cwd and path.root would've been the same path in previous Terraform versions, so perhaps using path.cwd instead of path.root is an answer that would work today, without any further Terraform changes. As long as you always cd into the directory containing the root module before running Terraform -- which is the most common usage pattern -- path.cwd should give the same path you had before.

@AndiDog
Copy link
Contributor Author

AndiDog commented May 23, 2019

path.cwd is a workaround, true, but might not be the expected path (in the end, it's only the "working directory", so semantically a different thing).

To clarify my use case again: I want to tag AWS resources in a way that others can find which Git repo + subdirectory the resource came from.

Example:

/Users/asommer/dev/my-git-repo-dir/staging/terraform/main.tf is the root module => resources should be tagged CreatedFromTerraformRoot=my-git-repo-dir/staging/terraform. I'm using the regex because this is the easiest way to strip the path until the repo name (which typically our developers won't rename to something else). pathrel would work if the Git top-level directory was somehow available, but that's seriously too much to ask as addition to Terraform.


I've added a PR for an abspath function which solves this case for me, and also is a good idea in general for whatever corner cases people might have.

aaron-lane added a commit to newcontext-oss/kitchen-terraform that referenced this issue May 25, 2019
Terraform 0.12 changed the behaviour of path.module to return a relative
pathname rather than an absolute pathname.

hashicorp/terraform#21400
aaron-lane added a commit to newcontext-oss/kitchen-terraform that referenced this issue May 25, 2019
* Update Gemfile.locks

* Add support for Terraform v0.12

* Add version constraints to attributes module

* Add version constraints to backend-ssh module

This module can not support Terraform 0.12 until the Docker provider
implements support.

hashicorp/terraform-provider-docker#144

* Add version constraints to Plug Ins module

* Add version constraints to variables module

* Update Terraform to 0.11.14 on Ubuntu

* Fix name of Ubuntu job

Terragrunt is not used on Ubuntu

* Update Terragrunt to 0.18.6 on MacOS

* Update local provider to 1.2.2

* Update Terraform to 0.12.0 on MacOS

* Add entry to CHANGELOG

* Update local provider version constraint for 0.12

* Replace path.module with path.cwd

Terraform 0.12 changed the behaviour of path.module to return a relative
pathname rather than an absolute pathname.

hashicorp/terraform#21400

* Declare terragrunt variables

Declaring these variables will suppress undeclared variable warnings.

* Remove escaped double quotes from string variable

Terraform v0.12 evaluates the quotes as part of the string.
@apparentlymart
Copy link
Contributor

Hi again!

Since your abspath PR was merged and included in today's release, I'm going to close this out now. Thanks for working on that PR, and for sharing the use-case.

@smurfralf
Copy link

smurfralf commented Jul 26, 2019

I find the justification for breaking backward compatibility presented in 2cf63d0 weak. IMO if a normalized-everywhere value was desired then it should have been added as an additional value and developer must bite the refactoring bullet and replace the internal uses of path.root with the new value.

@ghost
Copy link

ghost commented Aug 11, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Aug 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants