Import functions from Terraform #37
Conversation
* copy pasted the missing funcs & tests from github.com/hashicorp/terraform/lang/funcs/ 39e609 * forced github.com/bmatcuk/doublestar to use v1.1.5 so that TestFileSet passes; v1.2 had a different behavior. * renamed terraform's ReverseFunc to ReverseListFunc
azr
force-pushed
the
import-terraform-funcs
branch
from
e51ae7c
to
571e23c
Compare
go.mod
Outdated
|
|
||
| go 1.13 |
There was a problem hiding this comment.
Ah; go added this for me but I'm not sure this is helpful here; should we drop it ?
There was a problem hiding this comment.
I believe this is used just to add an extra note to error messages prompting that they might be caused by the requirement of a new Go version. For Terraform's sake this codebase is still targeting Go 1.12 compatibility, so perhaps we could explicitly set this to go 1.12 so we're clearer about that.
I think Go 1.13 will not try to update it if it's already there, but that this behavior is just to encourage adding a version specifier to improve the UX for future Go language changes.
There was a problem hiding this comment.
Got it; setting this to 1.12 for now 🙂
go.mod
Outdated
| github.com/apparentlymart/go-textseg v1.0.0 | ||
| github.com/golang/protobuf v1.1.0 // indirect | ||
| github.com/bmatcuk/doublestar v1.1.5 |
There was a problem hiding this comment.
v1.2 of this pkg has a different behaviour; this causes the tests to fail for older versions of go (1.11 & 1.12) :
--- FAIL: TestFileSet (0.00s)
--- FAIL: TestFileSet/FileSet(".",_cty.StringVal("."),_cty.StringVal("\\")) (0.00s)
filesystem_test.go:297: succeeded; want error
There was a problem hiding this comment.
I'd like to fix this test and use v1.2 on go1.13 but I'm not sure if this will be a breaking change ? And if so if that's going to be an issue. I think it should be fine 🤔
There was a problem hiding this comment.
I'm not sure I fully understand the implications of this change in behavior right now. Could we leave this on v1.1.0 for the moment and consider the implications of upgrading this separately?
Terraform will be using Go 1.12 until its next major release, so in particular we need to be careful here not to do anything that would cause misbehavior under Go 1.12.
There was a problem hiding this comment.
Ah it turns out a go mod tidy removed it. it's in hashicorp/go-cty-funcs#4 now !
There was a problem hiding this comment.
Thanks for working on this, @azr! I'm broadly on board with the idea of bringing most of these in. I have some concerns which I think can all be placed into the following three general categories:
- Weird functions that Terraform has for historical reasons but that are either now unnecessary due to being replaced by other language features or that are still necessary but have a strange API I don't like due to historical decisions.
- Terraform's set of "impure" functions that don't comply with the expected contract that a function should produce a result based only on its arguments and no other ambient state. As I noted inline, Terraform treats these as special during its plan phase to work around them being odd, but I don't want to introduce them into the stdlib and encourage their use in other software that doesn't have that special handling.
- Functions that are useful to more than just Terraform but that are still rather use-case specific, like the CIDR address manipulation functions, the cryptographic functions, and the filesystem functions. For these I'm not opposed to them being in
ctyin principle, but I have concerns about continuing to grow the scope of thisstdlibwhich lives in what is my personal project and thus, in the end, is my personal responsibility to maintain rather than a shared HashiCorp responsibility.
Most of my inline comments are special cases of these, but I didn't comment every example because that seemed like overkill.
With that said, I'd like to propose a compromise:
- First, I'd like to put the CIDR manipulation functions, the cryptographic functions, and the filesystem-interacting functions each in a separate codebase that can then be imported selectively by applications that need them. I don't really mind whether these separate codebases live here in my
zclconforganization or in thehashicorporganization, but Iean more towards the latter because then you'll be free to make HashiCorp-specific decisions (like using HashiCorp's non-standard UUID library) without me being "difficult" about them, and also it'll be clearer that they are fair game to be maintained by anyone at HashiCorp, rather than everything going through me. - For the "weirdo" functions that I indicated Terraform only has for backward-compatibility, I'd recommend not including them in other applications at all. If it's desirable to include them for some reason in spite of there being first-class HCL language features with the same functionality, I'd just copy-paste them into the other application because there aren't very many of them and they are very unlikely to change due to their role as backward-compatibility shims anyway.
- All but one of the "impure" functions fit into the cryptographic and filesystem categories I covered already anyway. The remaining one is
timestamp, and I think it would be overkill to establish a new codebase just for that so maybe again it's sufficient to just copy-paste it into another application that needs it, as long as that application is prepared to deal with a function that doesn't return a stable result.
Sorry for being a bit difficult here. This being my personal project codebase puts me in a weird position here cause the me wearing my HashiCorp employee hat wants to help you get this done but the individual me who will be primarily maintaining this codebase moving forward wants to make sure it doesn't grow to include things that are likely to make that harder. I hope the proposed compromises seem okay; if you'd like to talk more synchronously about other tradeoffs we could make I'm happy to.
|
Hey @apparentlymart; thanks a lot for taking the time to review; 🙂 I totally agree with you that there should probably be a library containing some hashicorp specific defined/maintained functions, also note that I would be very happing to help you maintaining this current project as it is and is going to be used by Packer a lot. Packer uses a lot of 3rd party projects and I usually try to help//fixing issues in there too. I'll check on the comments now ! Thanks again ! |
Codecov Report
@@ Coverage Diff @@
## master #37 +/- ##
==========================================
- Coverage 76.08% 69.57% -6.52%
==========================================
Files 77 78 +1
Lines 6172 6964 +792
==========================================
+ Hits 4696 4845 +149
- Misses 1060 1692 +632
- Partials 416 427 +11
Continue to review full report at Codecov.
|
as they are non deterministic
…llFunc * so that the decision on wether to use a regular expression falls upon the caller and not a heuristic * and the caller can decide the number of times replace is done
Quoting our discussion: zclconf#37 (comment) This one doesn't feel like a good fit for cty stdlib because it makes some assumptions about how we deal with the filesystem that are unlikely to be a good fit for all callers. If this were to be in cty we'd want to think about ways to let the calling application have more control over how filenames are resolved so that e.g. it could be safe to use the filesystem-related functions in an app with different security concerns, but that would likely make it a lot more complicated. So this will be done in another PR.
|
I'm thinking of creating a github.com/hashicorp/go-cty project; that will contain the following folders containing their own go module: function/cidr Edit: we picked https://github.com/hashicorp/go-cty-funcs |
* they would be a lot to maintain or are not canonical functions
There was a problem hiding this comment.
Het @apparentlymart; I think I applied all the needed changes 🙂 tell me what you think
cty/function/stdlib/encoding.go
Outdated
| @@ -0,0 +1,140 @@ | |||
| package stdlib | |||
There was a problem hiding this comment.
About this file; do you think I should remove any function in it ?
If not, what about Base64Sha256Func, Base64Sha512Func ? Should I put them in here as well ?
There was a problem hiding this comment.
There is some precedent for encoding functions in this package (e.g. JSONEncode, CSVDecode, etc), but the ones there so far were motivated primarily by them being intuitive encodings of the cty data types, and in most cases also just wrappers around functionality that's already in this codebase anyway. I'd also previously drawn the line of not bringing YAML encoding in here and putting that in a separate codebase.
For that reason, my instinct is to put the hashing functions alongside the other "crypto" stuff, and also to put the other encoding-ish things in a separate place because they aren't really cty-type-specific sorts of things, but are rather motivated by use-cases inside Terraform and now Packer.
There was a problem hiding this comment.
Got it; I understand and agree; I think I'll also create an encoding pkg 🤔
There was a problem hiding this comment.
Thanks for being flexible, @azr! I've replied to your questions inline and also on a second read I spotted a couple things I didn't catch on the first read through... sorry about that.
This looks close though! Thanks again for working on this.
cty/function/stdlib/collection.go
Outdated
| if err != nil { | ||
| // Should never happen since we checked this in our | ||
| // type-checking function. | ||
| return cty.NilVal, fmt.Errorf("failed to convert argVals[%d][%d] to %s; this is a bug in Terraform", j, n, ty.FriendlyName()) |
There was a problem hiding this comment.
I think we'll need to update this error message to put this in cty. 😉
I think let's just change it to "this is a bug" and let the user infer what it's a bug in depending on the context. Of course this is a message we don't intend users to ever see, so it doesn't matter too much.
(Sorry I didn't catch that on the first read)
There was a problem hiding this comment.
Oh good catch; changing this.
cty/function/stdlib/collection.go
Outdated
| argTy := arg.Type() | ||
|
|
||
| if argTy.IsSetType() { | ||
| return cty.NilType, function.NewArgErrorf(0, "cannot slice a set, because its elements do not have indices; use the tolist function to force conversion to list if the ordering of the result is not important") |
There was a problem hiding this comment.
This error message is now a bit tricky, cause it's assuming that any application which has slice exposed also has tolist exposed, and is calling it tolist.
It's a bit of a UX regression cause the message is then a little less actionable, but maybe we can change it to just say "explicitly convert to a list if the ordering of the result is not important" for now and look for a better answer in a later change. 🤔
cty/function/stdlib/encoding.go
Outdated
| @@ -0,0 +1,140 @@ | |||
| package stdlib | |||
There was a problem hiding this comment.
There is some precedent for encoding functions in this package (e.g. JSONEncode, CSVDecode, etc), but the ones there so far were motivated primarily by them being intuitive encodings of the cty data types, and in most cases also just wrappers around functionality that's already in this codebase anyway. I'd also previously drawn the line of not bringing YAML encoding in here and putting that in a separate codebase.
For that reason, my instinct is to put the hashing functions alongside the other "crypto" stuff, and also to put the other encoding-ish things in a separate place because they aren't really cty-type-specific sorts of things, but are rather motivated by use-cases inside Terraform and now Packer.
go.mod
Outdated
| github.com/apparentlymart/go-textseg v1.0.0 | ||
| github.com/golang/protobuf v1.1.0 // indirect | ||
| github.com/bmatcuk/doublestar v1.1.5 |
There was a problem hiding this comment.
I'm not sure I fully understand the implications of this change in behavior right now. Could we leave this on v1.1.0 for the moment and consider the implications of upgrading this separately?
Terraform will be using Go 1.12 until its next major release, so in particular we need to be careful here not to do anything that would cause misbehavior under Go 1.12.
go.mod
Outdated
|
|
||
| go 1.13 |
There was a problem hiding this comment.
I believe this is used just to add an extra note to error messages prompting that they might be caused by the requirement of a new Go version. For Terraform's sake this codebase is still targeting Go 1.12 compatibility, so perhaps we could explicitly set this to go 1.12 so we're clearer about that.
I think Go 1.13 will not try to update it if it's already there, but that this behavior is just to encourage adding a version specifier to improve the UX for future Go language changes.
cty/function/stdlib/datetime.go
Outdated
| }, | ||
| Type: function.StaticReturnType(cty.String), | ||
| Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { | ||
| ts, err := time.Parse(time.RFC3339, args[0].AsString()) |
There was a problem hiding this comment.
This is specifically using RFC3339; do you want me to make this one a MakeTimeAddFunc(layout string) to allow/force a user to pick a layout ?
There was a problem hiding this comment.
The formatdate function that's already in here has created the precedent that RFC3339 is the canonical time format for cty, so I think it makes sense to continue that precedent here. Users that need a different date format as output can pass the result to formatdate to change it.
This convention that timestamps are just strings written in RFC3339 notation actually originated in Terraform itself prior to cty existing, but I just embraced it here since it seemed like a good enough idea. I wanted to point that out because that is a Terraform-wide convention: provider plugins are expected to produce and accept timestamps in RFC3339 format, even if that means that the provider has to do conversions itself from whatever representation a remote API is using.
Hopefully Packer would be able to dictate a similar dictum so that everything will compose together well, but I don't know if there's existing precedent in Packer for timestamps in other formats. If so, Packer might end up wanting its own time-manipulation functions in order to work well with the existing conventions. I'd prefer to keep the cty convention simple either way, and let individual applications handle their own differing conventions if needed.
There was a problem hiding this comment.
Looking at formatdate again reminded me that this package already has a parseTimestamp function which wraps time.Parse(time.RFC3339, ...) to produce better error messages. It'd be nice to use that here; I think it should be a drop-in replacement.
There was a problem hiding this comment.
Got it, and I agree. I think I want Packer to do the same thing to get a similar UX.
I just updated formatdate !
| // units are `ns`, `us` (or `µs`), `"ms"`, `"s"`, `"m"`, and `"h"`. The first | ||
| // number may be negative to indicate a negative duration, like `"-2h5m"`. | ||
| // | ||
| // The result is a string, also in RFC 3339 format, representing the result |
There was a problem hiding this comment.
Ditto about the RFC3339 format
azr
force-pushed
the
import-terraform-funcs
branch
from
a5a6ca1
to
8e00637
Compare
|
Okay @apparentlymart; I [re]moved all what needed to be removed ! This cleaned up go.mod for us 🙂. The PR is much smaller; and lots of it is now hashicorp/go-cty-funcs#4 |
There was a problem hiding this comment.
Sorry for the delayed reply, @azr! I've been on vacation. 🏖️
Thanks for all of the updates. I ended up spotting one more thing 🙄 when considering your question about time formats but aside from that small thing I think this is good to go!
cty/function/stdlib/datetime.go
Outdated
| }, | ||
| Type: function.StaticReturnType(cty.String), | ||
| Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { | ||
| ts, err := time.Parse(time.RFC3339, args[0].AsString()) |
There was a problem hiding this comment.
The formatdate function that's already in here has created the precedent that RFC3339 is the canonical time format for cty, so I think it makes sense to continue that precedent here. Users that need a different date format as output can pass the result to formatdate to change it.
This convention that timestamps are just strings written in RFC3339 notation actually originated in Terraform itself prior to cty existing, but I just embraced it here since it seemed like a good enough idea. I wanted to point that out because that is a Terraform-wide convention: provider plugins are expected to produce and accept timestamps in RFC3339 format, even if that means that the provider has to do conversions itself from whatever representation a remote API is using.
Hopefully Packer would be able to dictate a similar dictum so that everything will compose together well, but I don't know if there's existing precedent in Packer for timestamps in other formats. If so, Packer might end up wanting its own time-manipulation functions in order to work well with the existing conventions. I'd prefer to keep the cty convention simple either way, and let individual applications handle their own differing conventions if needed.
cty/function/stdlib/datetime.go
Outdated
| }, | ||
| Type: function.StaticReturnType(cty.String), | ||
| Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { | ||
| ts, err := time.Parse(time.RFC3339, args[0].AsString()) |
There was a problem hiding this comment.
Looking at formatdate again reminded me that this package already has a parseTimestamp function which wraps time.Parse(time.RFC3339, ...) to produce better error messages. It'd be nice to use that here; I think it should be a drop-in replacement.
|
I also just noticed that the |
|
Hey @apparentlymart, thanks for reviewing again, no worries the more the fixes the better ! 😄 I hope your holidays were enjoyable and resting !! |
|
Thanks @azr! Sorry for all the back and forth here. I'm going to merge this now. |
|
Hey @apparentlymart no worries at all ! It was a wide scope. Thanks for bearing with me 🙂 |
Howdy, 🙂 this imports all functions from Terraform's
lang/funcs/package; I checked them and I think all func from there are standard enough; except for the funcs unsingMakeTemplateFileFuncthas does specific hcl2 things.