Import functions from Terraform #4
Conversation
| tests = append(tests, []testCase{ | ||
| { | ||
| cty.StringVal("."), | ||
| cty.StringVal("//"), | ||
| cty.SetValEmpty(cty.String), | ||
| false, | ||
| }, | ||
| }..., | ||
| ) |
There was a problem hiding this comment.
Drawing your attention to this case ! This passes but the behaviour is different on unix filesystems it seems. This sound good to me there might be implications I don't know of.
There was a problem hiding this comment.
In Terraform's case there is a convention that filesystem paths are always represented with forward slashes rather than backslashes so that behavior can be consistent even on Windows... Windows supports both backslashes and forward slashes, as long as a given path string is consistent about which it is using.
I'm not sure how best to map that requirement into this generalized library. Because Terraform is primarily concerned with managing remote objects it makes sense for it to try to abstract away details about the machine where Terraform itself is running, but that may be less appropriate for other software.
For this difference in particular: I'm not sure exactly what's going on here, but the intent is for this function to produce the same result regardless of host platform, and the presence of this special case does seem to suggest that it's not succeeding in that goal. If we want to preserve Terraform's assumptions then it would be nice to fix this here so that Windows behaves identically to others. We could also choose to go the other way and say that this function should always use conventions that make sense for the current host platform. But an "in-between" situation where it's mostly but not quite platform-agnostic doesn't seem like a good state to be in. 😕
There was a problem hiding this comment.
This looks broadly good to me.
The decision to make this repository contain multiple independent modules is interesting. I'm not opposed to it on principle, but I want to note that it will require separate tagging for versions of each module rather than a single tag for the whole repository. That is, instead of a single v1.0.0 tag we'll need separate cidr/v1.0.0, crypto/v1.0.0, encoding/v1.0.0, etc tags.
This could make sense because I do expect they will tend to evolve separately rather than together, but I think it's not a workflow we have a lot of precedent for in other Go library repositories in the hashicorp organization and I think it would be a difficult decision to reverse later if we change our minds (because Go Modules has rough edges switching between these two modes), so might be worth seeking some broader consensus on this from other teams whose codebases might import this one (maybe Nomad in future, for example?) before moving forward, rather than just me. 😁
I do particularly like how separating them makes the dependency set for each one lighter, which could be useful if we expect that applications might import only a subset of these modules.
…hicorp/go-uuid".GenerateUUID
This reverts commit 6f9b6d7.
|
Martin, thanks for reviewing, I'll write up an RFC :D |
|
Hello there, so this RFC was written for another project where it makes more sense to split submodules and while splitting sub packages into submodules is a good idea, I think ( and we have agreed offline) that it constitutes sort of premature optimisation here ! So I'll put everything back together and make this one pkg ! |
There was a problem hiding this comment.
Hi @azr! I'm sorry I lost track of this PR for a bit after some other work came up. I agree that having it all as one module feels more straightforward and should be sufficient for now, given that these things were all okay being together in a single module in Terraform before anyway.
|
Hey @apparentlymart ! Thanks for reviewing, no worries ! I'm going to go ahead and merge this to, Packer - HCL has been using this for a while now. I will not tag this repo yet to give it sort of a cool down period, just in case we need to break something. |
This imports some Terraform functions for cross organisational usage ! I didn't import base64sha256, base64sha512,filebase64sha256, filebase64sha512,filemd5,filesha1, filesha256, filesha512 because they are regroupement of functions and don't really fit in a pkg easily. I think they should copy pasted instead and an in-between variable could be used instead ?
I also didn't copy base64gzip because it seems super specific to Terraform