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

lang/funcs: Add fileset function #22523

Merged
merged 1 commit into from
Aug 26, 2019
Merged

lang/funcs: Add fileset function #22523

merged 1 commit into from
Aug 26, 2019

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Aug 20, 2019

Reference: #16697

Enumerates a set of regular file names from a given glob pattern.

Example Usage:

> fileset("${path.module}/*.txt")
[
  "path/to/module/hello.txt",
  "path/to/module/world.txt",
]
resource "aws_s3_bucket_object" "example" {
  for_each = fileset("${path.module}/*.txt")

  bucket = aws_s3_bucket.example.bucket
  key    = replace(each.value, "${path.module}", "example-prefix")
  source = each.value
}

Potential Caveats:

  • Implemented via the Go stdlib 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 code
  • Implemented as fileset using cty.Set(cty.String) for easier usage with for_each, but could just as easily be filelist using cty.List(cty.String) type
  • Includes relative path information from base directory in each result by default

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.
@tkellen
Copy link

tkellen commented Aug 24, 2019

🎉 Thank you so much! I would love to use this in conjunction with the new resource level for_each construct to automatically create ECR repositories on AWS by scanning a directory containing one folder per container.

@pselle pselle requested a review from a team August 26, 2019 16:10
Copy link
Contributor

@pselle pselle left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

V helpful comments here ❤️

@pselle pselle requested a review from a team August 26, 2019 16:13
Copy link
Contributor

@mildwonkey mildwonkey left a 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!

@pselle pselle merged commit cf687a9 into master Aug 26, 2019
Copy link
Contributor

@apparentlymart apparentlymart left a 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)
Copy link
Contributor

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",
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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.

@bflad bflad deleted the f-lang-funcs-filelist branch August 28, 2019 14:15
bflad added a commit that referenced this pull request Aug 28, 2019
…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.
bflad added a commit that referenced this pull request Aug 31, 2019
…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.
@ghost
Copy link

ghost commented Sep 26, 2019

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 Sep 26, 2019
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.

5 participants