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

CopyFile: support assets, archives, and recursive copying #423

Merged
merged 23 commits into from
Jun 20, 2024

Conversation

thomas11
Copy link
Contributor

@thomas11 thomas11 commented Apr 30, 2024

This PR allows the CopyFile resource to recursively copy directories as well, similar to scp -r.

This PR enhances the CopyFile resource in a few ways.

  • It can now copy directories as well, recursively with their contents.
  • It now takes the standard Pulumi asserts and archives as input, allowing for seamless interop with other resources.
  • It now checks whether the specified file or directory have changed (in content, not timestamp) and copies only if they did.

In light of these changes, I've renamed CopyFile to just Copy which is a breaking change.

Resolves #23
Resolves #33
Resolves #42

TODO

  • The current implementation fails as soon as a file or directory already exists on the remote, like the previous CopyFile.
  • Before GA, should we rename CopyFile to Copy or RemoteCopy or insert your proposal here?
  • The integration test copies a bunch of EC2 setup code with TestEc2RemoteTs, which we should share instead. What would be a good way to do that, given the code is in the TS tests, not in the Go driver?

Design considerations

The behavior of the copy operation was modeled after cp and scp.

source | dest - exists as dir | dest - does not exist | dest - exists as file
-------|----------------------|-----------------------|-----------------------
dir    | dest/dir             | dest/dir              | error
dir/   | dest/x for x in dir  | dest/dir              | error
file   | dest/file            | dest                  | dest (overwritten)

Specifically:

  • When copying a directory, we overwrite existing files. (The current CopyFile resource for single files does that, too.)
  • When copying a directory, we don't clear the remote directory first so that no left-over files from previous copies will be around. We can always add a flag for that if needed.

Implementation notes

  • I've looked around for open source Go packages implementing scp of folders, but to my surprise, I couldn't find one with an acceptable license that 1) wasn't ancient and 2) was cross-platform.
  • The current implementation copies files sequentially and is therefore probably slow for large trees. If we replaced the implementation, I don't think the shape of the resource would change, so this shouldn't be a GA blocker.
  • The size.ts file in both steps of the integration test is useless because it always has the same value. However, I found that without a TS file present in the additional step, it would not be run. Haven't debugged further.

@thomas11 thomas11 force-pushed the tkappler/archive branch 5 times, most recently from 1d6106b to 96cfa51 Compare May 6, 2024 14:19
@thomas11 thomas11 requested a review from a team May 7, 2024 07:38
@thomas11 thomas11 force-pushed the tkappler/archive branch from 96cfa51 to 9918546 Compare May 7, 2024 09:52
@thomas11 thomas11 force-pushed the tkappler/archive branch 2 times, most recently from ee19349 to c8c200f Compare May 13, 2024 14:40
@thomas11 thomas11 force-pushed the tkappler/archive branch from a919ac1 to 33e531d Compare May 14, 2024 13:01
@thomas11 thomas11 force-pushed the tkappler/archive branch from 33e531d to 0b11dc3 Compare May 14, 2024 18:09
@thomas11 thomas11 force-pushed the tkappler/archive branch 5 times, most recently from 72c9b7b to 4da0d5e Compare May 22, 2024 14:04
@thomas11 thomas11 requested review from danielrbradley and a team May 22, 2024 17:41
Copy link
Member

@iwahbe iwahbe 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 pretty good to me.

@@ -469,22 +469,22 @@
"connection"
]
},
"command:remote:CopyFile": {
Copy link
Member

@danielrbradley danielrbradley Jun 14, 2024

Choose a reason for hiding this comment

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

Should we include an alias for this rename? Probably not too significant because this resource doesn't really use state in any meaningful way, but might be nicer to avoid a delete/create on the next deploy after upgrading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to do some testing around this. It's not only the name of the resource that changes, the main input property for the source of the copy changes as well. I think a replace might be unavoidable.

Copy link
Member

Choose a reason for hiding this comment

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

I think you'll be okay as that property doesn't cause replacements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another idea was to keep the CopyFile resource as-is, alongside the new Copy resource, with a deprecation warning, until v2 of the provider.

@thomas11 thomas11 force-pushed the tkappler/archive branch 3 times, most recently from 4228300 to d96ea8f Compare June 19, 2024 14:45
@thomas11 thomas11 merged commit 40d4ab3 into master Jun 20, 2024
17 checks passed
@thomas11 thomas11 deleted the tkappler/archive branch June 20, 2024 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants