-
Notifications
You must be signed in to change notification settings - Fork 47
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
ignore excluded paths when transferring files #163
base: main
Are you sure you want to change the base?
Conversation
Hey @lbajolet-hashicorp sorry for directly pinging, but this PR was open for a while I wanted to see if I can get any help from you on this. I have tested the this on the aws plugin and it works I'm not sure where can we add the acceptance for this as there's no acceptance test on the repo. Thanks for your time. |
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.
Hi @Glyphack,
Sorry I took so long to take a look at this PR, this slipped my mind, apologies again.
Taking a look at the code, the approach looks OK to me, about the excludes
attribute that you added to the file
provisioner, I believe this would be better as part of the communicator config directly, so that it will work automagically for all the plugins that update their SDK dependency.
In addition to this, and I know that in this repo we don't have that setup since it relies on plugins to test, but I think it would be great to have some acceptance tests to make sure the excludes option works in all cases.
While working on #180, I wrote a series of acceptance tests for the scp
and sftp
copies, it's not public yet, but if you're working on this code as well, I can maybe give you access to this repository so you can base on that to work on acceptance tests for this feature.
This requires a bit of fiddling since both Packer
and the qemu
plugins need to be compiled against an up-to-date sdk for the tests to work, but if necessary we can help you on this.
Let me know if that would interest you, otherwise I can also write some tests and run them against this PR.
Thanks again for the proposal, it's an interesting feature which I'm sure will be helpful to many users.
testCases := []struct { | ||
Exclude []string | ||
Path string | ||
Is bool |
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 terms of clarity, this bool seems to be used to check that we did exclude something, could you rename it to make it clearer what it's used for?
That's personal preference, but I found that constructions like ExpectExclude
works well at communicating the intent, but feel free to give it your own take on that!
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.
Nice suggestion, applied!
Just wanted to let you know that I have this in my radar, and will get back to it next week latest. In the meanwhile I can get some help on how to continue with updating [winRM] (hashicorp/packer#12280 (comment)) uploader. |
Thanks for the comments @lbajolet-hashicorp, I've applied them the only one which I could not address was winRM for the reason I mentioned in previous comment. Regarding tests, I'm interested in learning it from you. Let me know when you have time to share the code or look into it together. |
any update on this? would be really nice to have |
This PR implements the exclude file feature related to this PR in packer repo.
While uploading directories each object is checked against exclude list, and if it's an exact match that object(file, symlink, directory) is skipped. The pattern matching is done using filepath.Match and I'm open to changing it to another syntax like regex.
I have wrote unit tests for it but for testing it completely I have used it on aws plugin to make sure it does not upload the file.
I'd be happy if you can provide some initial feedback on the approach to resolve the issue.