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

config/module: storage path should be independent of system #1418

Merged
merged 4 commits into from
Apr 8, 2015

Conversation

mitchellh
Copy link
Contributor

This is a really important change to make copying entire .terraform directories work properly. Before, the hash in the .terraform/modules folder would be calculated based on the full URL of the source of the module. There are benefits to this:

  1. One copy per source
  2. When the source changes, terraform immediately notices.

The downside is that the paths aren't portable at all, which conflicts with terraform push. In this PR, I change the hash to be the hash of the "path" of the module: example "root.child.child2". This makes it portable, but makes it so that we have an exact 1:1 mapping of download of a module to folder. I don't think this optimization matters right now.

I think the proper long term solution is to have some sort of "module.lock" file that does this mapping for us so the hash doesn't matter at all. But for a 0.4.1 I wanted to minimize what is touched.

@phinze: Would appreciate some thoughts here and a review.

@phinze
Copy link
Contributor

phinze commented Apr 8, 2015

These two args should change from source -> key I think?

func (s *FolderStorage) Dir(source string) (d string, e bool, err error)
func (s *FolderStorage) dir(source string) string

@mitchellh
Copy link
Contributor Author

Yep.

@phinze
Copy link
Contributor

phinze commented Apr 8, 2015

Ok, I've grokked the config/module package now. For some reason I found it super-difficult to sort out. Maybe because relatively terse stuff like Get and Dir have very special meanings. Now that I get it, I wish I had a recommendation for a clarity refactor, but of course I'm browsing it and it all makes sense now. 😝 Anyways, maybe next time I'm in the area.

@phinze
Copy link
Contributor

phinze commented Apr 8, 2015

Ok two questions:

Before, the hash in the .terraform/modules folder would be calculated based on the full URL of the source of the module... the downside is that the paths aren't portable at all

Why weren't the paths portable? Seems like an MD5 of an absolute URL should result in a consistent hash?

This makes it portable, but makes it so that we have an exact 1:1 mapping of download of a module to folder.

I think you meant "no longer have" here yeah? So multiple instances of a module means it could be downloaded twice.

@mitchellh
Copy link
Contributor Author

That's right. The URL for file paths was specific to a system, so it wasn't portable.

mitchellh added a commit that referenced this pull request Apr 8, 2015
config/module: storage path should be independent of system
@mitchellh mitchellh merged commit 3a3cae9 into master Apr 8, 2015
@mitchellh mitchellh deleted the b-module-storage-path branch April 8, 2015 00:56
rowleyaj added a commit to Ensighten/terraform that referenced this pull request Mar 11, 2016
An md5 hash of the dst is used as the folder name on disk.

In hashicorp#1418 this was altered
to not be platform specific. The dst was changed from the source to a
key based on the module path. This was done so that terraform push
would work. This had the side affect that modules with the same source
would result in multiple downloads. This was acknowledged in the original
commit.

This is a simple way to have the key be based on source again.

Tested with terraform get and all works ok. With terraform get --update
the update is done multiple times as if the 1:1 mapping was still in
place.
@ghost
Copy link

ghost commented May 3, 2020

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 May 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants