Follow symlinks when creating archive #183
Conversation
Co-authored-by: moajo <mimirosiasd@gmail.com>
bendbennett
force-pushed
the
bendbennett/issues-6
branch
from
1a1537d
to
ac273ea
Compare
There was a problem hiding this comment.
Approving but mainly to get eyes back on this (since I'm not an approving reviewer).
@bendbennett looks like it timed out on the Tests / Acc. Tests (OS: windows-latest / TF: 0.14.*) (pull_request) test after 15 mins.
internal/provider/zip_archiver.go
Outdated
| @@ -137,11 +144,29 @@ func (a *ZipArchiver) ArchiveDir(indirname string, excludes []string) error { | |||
| return err | |||
| } | |||
|
|
|||
| if info.Mode()&os.ModeSymlink == os.ModeSymlink { | |||
| realPath, err := readSymlinkRecursive(path, maxRecursion) | |||
There was a problem hiding this comment.
I commented above about the maxRecursion variable and decided to do a little more digging and found that the standard library has a function filepath.EvalSymlinks that, after a quick glance, looks very similar to this readSymlinkRecursive function (with some additional OS specific edge-cases to boot).
We may want to consider using this standard library function if it is providing the same functionality.
Fun fact, the magic number in the standard library is 255 symlinks! 😆
There was a problem hiding this comment.
Sweet! Nice find, thanks. I've refactored to switch to using filepath.EvalSymlinks. Looks like the functionality meets our requirements and it handles symlinks to absolute paths correctly out of the box. 👍
There was a problem hiding this comment.
We have some unit tests below for symlinks, but do we also want to add a scenario to the data source and resource tests here for symlinks?
There was a problem hiding this comment.
Good call! Have added a couple of tests for the archive_file resource and data source. These are currently leveraging helper functions (i.e., ensureContents() and ensureFileMode()) that were created for use with unit tests. Using these helper functions within acceptance tests looks a little strange as we're passing t *testing.T into the function. Happy to refactor the helper functions to just return an error and then update the unit tests and acceptance tests accordingly. Let me know your thoughts.
There was a problem hiding this comment.
I don't have any strong feelings for refactoring vs. not, I think what you have looks good 👍🏻
bendbennett
force-pushed
the
bendbennett/issues-6
branch
from
126ad62
to
0fe3cb8
Compare
bendbennett
force-pushed
the
bendbennett/issues-6
branch
from
4eb0df4
to
b6d685e
Compare
| var fileSize string | ||
| r.ParallelTest(t, r.TestCase{ | ||
| ProtoV5ProviderFactories: protoV5ProviderFactories(), | ||
| Steps: []r.TestStep{ |
There was a problem hiding this comment.
Would this testing be a little easier to read/grok/parallelize if each test step was treated as its own unit test? This would help ensure there is a separate t.TempDir() for each archive and there could be code comments explaining what should (or shouldn't) be expected for each test.
There was a problem hiding this comment.
Sounds like a good idea. Have separated each of the test steps into an individual test and added comments to clarify the expectations.
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Closes: #6
@moajo as the archive provider has now been migrated from SDKv2 to the Terraform Plugin Framework, I have incorporated your changes from #73 into this PR and added you as a coauthor on the commit as the vast majority of the changes were in your original PR. The only significant change I have made was in the handling of symbolic links that are using an absolute path. If you would rather rebase your original PR on the latest main then please go ahead and I will close out this PR. Thank you very much for your contribution.