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

[WIP] file: use excludes list to excl files when uploading a directory #12280

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Glyphack
Copy link

@Glyphack Glyphack commented Feb 18, 2023

Adds excludes configuration for file provisioner to ignore paths from a directory while uploading.

The implementation for excluding files during upload are in this PR

I will update the documents once the implementation details are agreed upon.

Question

I initially wanted to implement this feature as described here, and realized the UploadDir signature accepts an exclude arg.

Now my question is should this be implemented in this project or in the packer-sdk, here and here?

Resolves: #9671

Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

Thanks for opening up this PR. Apologies for taking so long to get back to you on it. Please let me know if you are still interested in working on this change.

provisioner/file/provisioner.go Show resolved Hide resolved
@@ -227,7 +232,7 @@ func (p *Provisioner) ProvisionUpload(ui packersdk.Ui, comm packersdk.Communicat

// If we're uploading a directory, short circuit and do that
if info.IsDir() {
if err = comm.UploadDir(dst, src, nil); err != nil {
if err = comm.UploadDir(dst, src, p.config.Excludes); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is needed in order to path in the list of files to exclude. But the actually implementation for supporting the excludes would need to be made in the Packer SDK communicator, which this line is calling out to. At this present time the exclude argument is ignored.

https://github.com/hashicorp/packer-plugin-sdk/blob/b03e2b6b7bbc38ca6b92770b33873860e796a5e0/sdk-internals/communicator/ssh/communicator.go#L187

https://github.com/hashicorp/packer-plugin-sdk/blob/b03e2b6b7bbc38ca6b92770b33873860e796a5e0/sdk-internals/communicator/winrm/communicator.go#L149

For the excludes we will need to provide guidance on the allowed paths and/or globs that can be passed into exclude, which I think the file provisioner could take on the work of expanding.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for noticing this. I already started a PR on packer-sdk to address this. I also left a comment there about how the pattern matching should be. Could you also take a look?

hashicorp/packer-plugin-sdk#163

@Glyphack
Copy link
Author

Hey @nywilken I'm absolutely interested to continue this. Don't need to apologize I totally understand you 😃 .

As you pointed out this change requires this packer-sdk PR to be merged. Do you think it's better to first merge that one and then complete the explanation for the exclude config based on the implementation in this PR?

Let me know what you think and I'm ready to apply changes.

@Glyphack Glyphack requested a review from nywilken April 12, 2023 19:03
@nywilken
Copy link
Contributor

@Glyphack thanks for updating the open PRs around this change. I got a little caught up with other priorities this week. I will follow up next week with feedback. Quickly looking at the SDK changes I would expect the same changes to exist for the WinRM communicator. But will have more details once I dive into the PR. Thanks again for this work and all the other contributions. They are much appreciated.

@nywilken nywilken added this to the 1.9.0 milestone May 1, 2023
@Glyphack
Copy link
Author

Glyphack commented May 5, 2023

I looked into adding this change for winRM. winrmcp project is used to copy the folder in this communicator, this dependency does not support excluding files, and it's not actively maintained.

I'm thinking one solution would be to not use the copy function and use write function to rewrite the walking over the directory part. Another way would be to remove this dependency. I'm not sure which one is better right now. What are your suggestions?

@nywilken nywilken removed this from the 1.9.0 milestone May 30, 2023
@nywilken nywilken added stage/waiting-on-upstream This issue is waiting on an upstream change stage/needs-discussion and removed upstream/packer-plugin-sdk labels Oct 17, 2023
@nywilken
Copy link
Contributor

Hi @Glyphack sorry, just sorry. We haven't had a chance to prioritize this change, and it doesn't help with the state of winrmcp at the moment. We are discussing this internally to see where we go from here with WinRM and testing things. You are welcome to keep this PR open or can close until we have a plan forward.

@Glyphack
Copy link
Author

Glyphack commented Oct 23, 2023

Hello @nywilken don't be sorry 😃, I understand that there are lots of issues, PRs, and beside that internal stuff so would not mind a late response. I would still like to work on it whenever you have chosen the solution.

@nywilken
Copy link
Contributor

Thank you @Glyphack - I appreciate your understanding and patience with these changes. I'll keep the PR open and ping you when we are ready to discuss next steps. Cheers!

@nywilken nywilken removed their request for review September 13, 2024 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow include/exclude for file provisioner
2 participants