-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add Remote FileCopy #21
Conversation
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.
A few comments from a first pass through.
"context" | ||
"os" | ||
|
||
"github.com/pkg/sftp" |
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.
Curious - as I'm not certain myself yet - but was there a reason to choose sftp over scp for this?
} | ||
|
||
func (c *remotefilecopy) RunDelete(ctx context.Context, host *provider.HostClient, urn resource.URN) error { | ||
host.Log(ctx, diag.Info, urn, "Deleting remotefilecopy") |
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.
I expect we don't need this?
(though we may benefit from debug level diagnostics in quite a few more places for future debuggability)
if err := inner(); err != nil { | ||
return "", err | ||
} | ||
return resource.NewUniqueHex("", 8, 0) |
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.
Any opportunity to add something more semantically meaningful here? I could image deriving a prefix from the remotePath potentially? But maybe more awkward than helpful?
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.
It would still need a random component because we might have an otherwise identical CopyFile
with a different connection
. I don't think there is much benefit. I think the same argument applies to remote:Command
.
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.
Yeah - agreed. I was wondering similar for remote Command, but there's even less sensible to use there. Sticking with the random hex sounds good.
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.
LGTM! I don't have any specific comments about documentation, either; everything looks nice and clean.
Adds a resource that represents moving a file from a local to a remote host.
Fixes #6