Infer destination filename when uploading to a directory (CLI level)#2970
Infer destination filename when uploading to a directory (CLI level)#2970Copilot wants to merge 1 commit into
Conversation
|
@copilot -- can you explain why the changes to web_ui/frontend/package-lock.json are necessary for this pull request? |
The package-lock.json changes were unintentional - they occurred when I ran |
bbockelm
left a comment
There was a problem hiding this comment.
- Drop the
package-lock.jsonchanges and don't bring them back in future commits- - I don't like changing the semantics of
DoPutat the library level - it feels like this is a CLI nicety. Can you useDoStatin the CLI instead and adjust the URLs to transfer there? You'd need to changecmd/object_put.goandcmd/object_sync.go. Do this in a helper function to simplify the addition of an integration test. Add an integration test.
I've addressed all requested changes:
Note: |
03bb1dd to
da08a35
Compare
|
@brianaydemir, @aowen-uwmad - can I get you to do a quick test of this? Particularly, I'm not sure how the behavior should interact with the In this case: it creates In this case: then it creates |
da08a35 to
5802a9f
Compare
|
@bbockelm, I'm not sure what you'd like me to "test". So I recreated the same movements using Case 1 Case 2 |
|
It doesn't look like there has been movement on this for ~3 weeks. The |
|
In particular, I'm hoping another developer can review the code, too. |
|
|
||
| // Helper function to get a temporary token file | ||
| func getTempTokenForTest(t *testing.T) (tempToken *os.File, tkn string) { | ||
| require.NoError(t, param.Set(param.IssuerKeysDirectory.GetName(), t.TempDir())) |
There was a problem hiding this comment.
Heads up, with recent changes to the param package, this should be updated to:
require.NoError(t, param.IssuerKeysDirectory.Set(t.TempDir()))
| defer os.Remove(tempToken.Name()) | ||
|
|
||
| // Disable progress bars | ||
| require.NoError(t, param.Set("Logging.DisableProgressBars", true)) |
There was a problem hiding this comment.
And this should be:
require.NoError(t, param.Logging_DisableProgressBars.Set(true))
Down with using strings to set params!
pelican object putnow behaves likecp/scpwhen the destination is an existing directory - it infers the filename from the source. This feature also handles multiple file uploads intelligently.Previously:
pelican object put ./file.txt osdf:///namespace # Error: remote object already exists, upload abortedNow:
Changes
Optimized directory check in
cmd/object_put.go: For non-recursive uploads, usesclient.DoStatonce before the upload loop to check if the remote destination is a directory. Within the loop, only filename transformations are performed, avoiding redundant stat calls.Multi-file upload support: When uploading multiple files to a destination that doesn't exist (stat fails), the code assumes the destination should be treated as a directory and infers filenames for each source file.
Implementation approach: Uses
client.DoStatto check if destination is a collection, then modifies the destination URL for each file before the actual transfer begins. Logic is implemented at the CLI level rather than in the library.Error handling: If URL parsing fails during filename inference, the operation terminates with a clear error message rather than silently continuing.
Integration test added:
TestObjectPutToDirectoryInfersFilenameincmd/object_put_test.goverifies that uploading to a directory correctly infers the filename from the source.This matches the existing
DoGetbehavior which already infers local filenames when downloading to a directory.Testing
Original prompt
pelican object putto "directory" location #2946✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.