rclone

rclone copied to clipboard

What is the purpose of this change?

Creates directory markers for Bucket Based Remotes to allow having empty folders on remote.

Usage: Adding --s3-directory-markers or --gcs-directory-markers flag will create and upload an empty object called <directory_name>/ every time a new folder is created. This allows for pseudo-directory tree to exist where "directories" can appear be empty, not only deriving folder structure from object names.The directory marker file (<directory_name>/) is filtered out from the file listing (existing functionality of rclone). This PR also removes marker files when the directory is removed (including recursive removal). (For context: this PR originally created object called .dirfile, but was changed to an object named / since it appears to be a more widely used approach.)

Was the change discussed in an issue or in the forum before?

Related issue: https://github.com/rclone/rclone/issues/3453

Checklist

• [X] I have read the contribution guidelines.
• [X] I have added tests for all changes in this PR if appropriate.
• [X] I have added documentation for the changes if appropriate.
• [X] All commit messages are in house style.
• [X] I'm done, this Pull Request is ready for review :-)

This isn't the accepted way of doing this.

The normal way is to create a zero sized object which ends in a /

These are already filtered out of directory listings.

Can you change the pr to do that? Thank you

Is there an example backend or other product where that method actually works? I noticed something similar in the codebase, but either i don't know how to enable it or it just does not work.

Is there an example backend or other product where that method actually works? I noticed something similar in the codebase, but either i don't know how to enable it or it just does not work.

I know Cyberduck makes them so it would be great to make rclone compatible with that.

Please run gofmt -w ./backend/s3/s3.go and commit its formatting changes. This will fix the unit test.

Thank You for checking the tests, @ivandeex, i'll fix them.

@ncw, I noticed that the file listing for directory objects is already implemented in rclone, but the part that creates those files is not. I replaced the directory marker filename with / and it works as expected now, but the directory removal part is broken since the trailing slash is trimmed off the path when Remove() is called. I'll fix the removal part and get back.

This is up for reviewing again. Seems to work as expected on S3/Minio and GCS

Suggested fixes done, custom API calls removed in favor of Put and Remove methods. The fix was adjusting split method to reattach the trailing slash if it was there before passing trough path.Join(f.root, rootRelativePath).

Took a while to get back to this, figure out tests and make relevant backend configurations for local tests, but it was well worth it as it caught one or two mistakes (i.e. s3 Rmdir didn't have DirectoryMarker conditional for the marker file removal).

All tests seem to pass in local now, Minio tests complain about TiersToTest, but AWS/S3 works as expected. I even tried enabling CanHaveEmptyDirectories feature for both backends, but then some errors popped up.

There is one thing I noticed, though - I might be doing something wrong, but tests don't seem to obey ExtraConfig, so i had to explicitly set option defaults value to true in code while running tests. This does not seem to do anything -

ExtraConfig: []fstests.ExtraConfigItem{
	{Name: name, Key: "directory_markers", Value: "true"},
},

Forgot to run the coding standards check gofmt -w, fix commited.

Of course I forgot to lint the test files themselves.. Is weird that it did not complain about markerFileObject.Remove(ctx) though.

please rebase on current master

:heavy_check_mark: Branch rebased on master, linter test rerun.

@Jancis thank you

@ncw your turn

I'm struggling with CanHaveEmptyDirectories, can't pass the FsMkdirRmdirSubdir test. go test -run=TestIntegration2/FsMkdir/FsMkdirRmdirSubdir -v fails with

expected: []string{"dir", "dir/subdir"}
actual  : []string{}

If I edit the test (fstests.go) and comment out the Rmdir(ctx, f, dir) that comes after directory creation, test leaves the bucket with files and the bucket contains correct expected directory structure, dir/subdir), it's just that the test does not pass for some reason. I tried to debug and see what other BBR's are doing about it, but i start to suspect they have similar issue (did not check each and every one of them), but at least swift backend fails exactly the same test when i enable that feature in it's code.

I'm stuck with this one, can we have CanHaveEmptyDirectories off for now like other BBR's have?

Current summary:

• Setting of CanHaveEmptyDirectories Feature flag if directory_markers is set.: Failed (see above)
• Fixing of integration test parameters so they work: Done
• Are you happy with this being squashed into 1 commit for merge to master or would you like more than one?: I'm absolutely fine with that, no need to keep the history.

Rebased on latest master to fix conflicts with DownloadURL changes.

Is there anything I can do for this PR, @ncw?

Please squash reasonably, merging 10 commits for single PR is too much :)

Commits reduced to 2, should combine them into one @ivandeex?

LGTM. Only @ncw review left.

just tested this, would love to see it merged <3

EDIT: shouldn't this be the default behavior?

I have rebased this. I've also fixed it up so that it works when the backend declares that it can support empty directories. This was a lot more work than I was expecting but it has made a nice result hopefully :-)