-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
lang/funcs: Add fileset function #22523
Conversation
Reference: #16697 Enumerates a set of regular file names from a given glob pattern. Implemented via the Go stdlib `path/filepath.Glob()` functionality. Notably, stdlib does not support `**` or `{}` extended patterns. See also: golang/go#11862 To support the extended glob patterns, it will require adding a dependency on a third party library or adding our own matching code.
🎉 Thank you so much! I would love to use this in conjunction with the new resource level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really cool and useful to me. I also appreciate it being a set. Requesting other core team review/eyes.
@@ -316,6 +370,16 @@ func FileExists(baseDir string, path cty.Value) (cty.Value, error) { | |||
return fn.Call([]cty.Value{path}) | |||
} | |||
|
|||
// FileSet enumerates a set of files given a glob pattern | |||
// | |||
// The underlying function implementation works relative to a particular base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
V helpful comments here ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that set
is the right default - users can always tolist()
the results if needed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Some minor usability/consistency feedback inline.
} | ||
|
||
// Ensure that the path is canonical for the host OS | ||
pattern = filepath.Clean(pattern) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of Terraform 0.12, we've standardized on always using forward slashes, even on Windows. While using the canonical form for the pattern is fine, we should use filepath.ToSlash
on the way out here for consistency with the other Terraform functionality that returns paths.
(This is so that you can write things like "${path.module}/${whatever}"
and get a working result on all platforms, whereas in Terraform 0.11 and earlier it would tend to produce broken results on Windows due to the mixture of slashes.)
return function.New(&function.Spec{ | ||
Params: []function.Parameter{ | ||
{ | ||
Name: "pattern", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In your example in the PR writeup you show having to replace the prefix with another prefix in order to compute the path on the S3 side.
With that use-case in mind, I'm thinking about whether this function should two arguments instead... The first argument would be a path to resolve the pattern relative to and the second would be the pattern, so then you could do something like this:
resource "aws_s3_bucket_object" "example" {
for_each = fileset(path.module, "*.txt")
bucket = aws_s3_bucket.example.bucket
key = each.value
source = "${path.module}/${each.value}"
}
Here I'm thinking that:
- The base directory argument is interpreted as a relative path from the
baseDir
, giving the matching base. - The pattern is interpreted as a relative path pattern from the matching base.
- The results are relative to the matching base.
Although it's a little more awkward to have to re-append the path.module
on source
here, adding that prefix back is perhaps easier and clearer than trying to replace it out and risk weird rough edges that might be hard to debug (like ../
paths, etc).
I expect this sort of "mirror all of these files onto some other system" is the main use-case for fileset
, so having this built into it will make that use-case easier to meet robustly. What do you think?
- `*` - matches any sequence of non-separator characters | ||
- `?` - matches any single non-separator character | ||
- `[RANGE]` - matches a range of characters | ||
- `[^RANGE]` - matches outside the range of characters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spelling out all the pattern match syntax here! It's nice to enumerate it, rather than presuming the user will have seen this syntax before.
] | ||
``` | ||
|
||
```hcl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't normally include full resource examples in the function docs without some additional context, so I think it could help to have a paragraph here describing what follows. For example...
A common use of `fileset` is to create one resource instance per matched file, using
[the `for_each` meta-argument](/docs/configuration/resources.html#for_each-multiple-resource-instances-defined-by-a-map-or-set-of-strings):
The main thing here is to link out to the for_each
documentation so that someone who isn't famililar with that feature can understand that it's the main thing we're trying to illustrate here and get some additional information on what the implications are of using it in this way.
…st argument, automatically trim the path argument from results, and ensure results are always canonical with forward slash path separators Reference: #22523 (review) These changes center around better function usability and consistency with other functions. The function has not yet been released, so these breaking changes can be applied safely.
…st argument, automatically trim the path argument from results, and ensure results are always canonical with forward slash path separators Reference: #22523 (review) These changes center around better function usability and consistency with other functions. The function has not yet been released, so these breaking changes can be applied safely.
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. |
Reference: #16697
Enumerates a set of regular file names from a given glob pattern.
Example Usage:
Potential Caveats:
path/filepath.Glob()
functionality. Notably, stdlib does not support**
or{}
extended patterns. See also: path/filepath: Glob should support**
for zero or more directories golang/go#11862 To support the extended glob patterns, it will require adding a dependency on a third party library (e.g. https://github.com/bmatcuk/doublestar) or adding our own matching codefileset
usingcty.Set(cty.String)
for easier usage withfor_each
, but could just as easily befilelist
usingcty.List(cty.String)
type