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

Use relative paths for archives #11

Merged
merged 3 commits into from
Jul 20, 2018

Conversation

raymondbutcher
Copy link

@raymondbutcher raymondbutcher commented Jul 18, 2018

what

  • Avoid using absolute paths which are not portable across workstations

why

This fixes issues when multiple people are working on the same project and they have the project located in different places.

Before this change, we were seeing plans like:

  ~ module.lambda_ami_backup.aws_lambda_function.ami_backup
      filename:                    "/home/user1/some-project/terraform/workspace/management-nonprod/.terraform/modules/49890e3df5623d30b47ac70e1936d38f/ami_backup.zip" => "/Users/user2/some-project/terraform/workspace/management-nonprod/.terraform/modules/49890e3df5623d30b47ac70e1936d38f/ami_backup.zip"

We've tested this change and the plans are now clean.

I've used relative paths like this in other modules/projects, putting archives in the .terraform directory, and haven't had any issues so far. An md5 is used in the filename to avoid conflicts with multiple versions of this module being used in the same project (unlikely but it's better to be safe).

@osterman
Copy link
Member

@raymondbutcher thanks for this fix! Makes sense. This was a bug =)

@osterman osterman requested review from osterman and aknysh July 19, 2018 03:31
@osterman osterman added the bug 🐛 An issue with the system label Jul 19, 2018
main.tf Outdated
@@ -47,13 +47,13 @@ data "aws_iam_policy_document" "ami_backup" {
data "archive_file" "ami_backup" {
type = "zip"
source_file = "${path.module}/ami_backup.py"
output_path = "${path.module}/ami_backup.zip"
output_path = ".terraform/terraform-aws-ec2-ami-backup/backup-${md5(file("${path.module}/ami_backup.py"))}.zip"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned with the portability of hardcoding both .terraform and the module name terraform-aws-ec2-ami-backup which is subject to change in forks.

I propose the following:

  • create a new variable called tfstate_dir and default it to .terraform
  • use a local called module_name which is derived from basename(path.module)
  • use a local to define an output_dir which is equal to ${tfstate_dir}/${module_name}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also a nice fix: hashicorp/terraform#8204 (comment)

Take your pick =)

Copy link
Member

@osterman osterman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline comments.

@raymondbutcher
Copy link
Author

Thanks @osterman, fair points.

I've done a (nicer IMO) variation of hashicorp/terraform#8204 (comment) that was suggested and this is the resulting plan:

  ~ module.lambda_ami_backup.aws_lambda_function.ami_backup
      filename: ".terraform/terraform-aws-ec2-ami-backup/backup-66f40c060f29561cb074987a03c45117.zip" => ".terraform/modules/49890e3df5623d30b47ac70e1936d38f/ami_backup.zip"

  ~ module.lambda_ami_backup.aws_lambda_function.ami_cleanup
      filename: ".terraform/terraform-aws-ec2-ami-backup/cleanup-ff78a3eaea904d4807a069837535acc0.zip" => ".terraform/modules/49890e3df5623d30b47ac70e1936d38f/ami_cleanup.zip"

As per my commit message:

This removes the assumption about Terraform making a .terraform directory in the current working directory, and removes the need to use md5 to handle multiple module versions in the same project as Terraform deals with that itself. It now assumes that the module is located within the current working directory.

I think it's safe to assume that. hashicorp/terraform#8204 (comment) makes the same assumption.

This fixes issues when multiple people are working on the same project and they have the project located in different places.
This removes the assumption about Terraform making a .terraform directory in the current working directory, and removes the need to use md5 to handle multiple module versions in the same project as Terraform deals with that itself. It now assumes that the module is located within the current working directory.
@raymondbutcher raymondbutcher force-pushed the ray/fix-absolute-paths branch from 0eccf57 to f0ba1b9 Compare July 20, 2018 11:00
@osterman
Copy link
Member

We have one other PR we should merge before this one. We will take care of this today.

@osterman osterman merged commit 25c71c3 into cloudposse:master Jul 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 An issue with the system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants