Check if folder exists before attempting to copy folders #743
Conversation
cbuto
changed the title
modules/files/files.go
Outdated
| return "", err | ||
| } | ||
| if !exists { | ||
| return "", os.ErrNotExist |
There was a problem hiding this comment.
NIT: Could you create a new error that includes the path of the non-existent folder in the error message? That'll help quite a bit with debugging.
There was a problem hiding this comment.
fixed this to include the folderPath. I wonder if I should also use that same error message here
There was a problem hiding this comment.
Yes please! In fact, we typically define a new type that implements the error interface (has an Error() method), so that (a) you can check if the error you got back was of a specific type, rather than doing string parsing and (b) you can re-use that error type in more than one place.
There was a problem hiding this comment.
got it, I created DirNotFoundError. Let me know if that's what you were thinking!
| t.Fatal(err) | ||
| } | ||
| if !exists { | ||
| t.Fatal(os.ErrNotExist, fullTerraformModuleFolder) |
There was a problem hiding this comment.
Will this show a clear error message?
There was a problem hiding this comment.
This will result in:
TestTerraformAwsExamplePlan: test_structure.go:82: file does not exist ../no/examples/terraform-aws-example
There was a problem hiding this comment.
That's decent, but would be slightly better if it said "directory" instead of file. If you extract + reuse the error type as mentioned above, that'll do the trick.
|
@brikis98 Thanks for taking the time to review this, I appreciate it! |
|
Ah, doh, I forgot we have a couple random test failures, and we're working on a fix now: #756. Let's get that merged first, and then I can re-run the tests here. |
|
Cool thanks! Let me know if I can help at all |
… failing faster and make debugging easier (files and test-structure)
|
I rebased this PR with master to pull in the test updates |
|
Thanks! I'll re-run tests now. |
This PR attempts to close #738.
I ran through the unit tests that were affected by these changes. I'm not sure if the output is super useful for those but I also ran some of the automated tests that use
CopyTerraformFolderToTempwhich look good. I was unable to run the full automated test suite right now, but I can look at doing at later if needed.Here is the output from the automated test I did run:
https://gist.github.com/cbuto/13dd83b40d29503288b11192ee2c4149