Update ListFiles to return directories and add MakeDir endpoint #4552

AdityaHegde · 2024-04-09T10:27:29Z

To prepare for asset explorer,

We need to get empty directories in ListFiles to allow users to add files to them.
We need a way to create a new directory. Adding MakeDir for this.

Note: We do not need this in watch files as of now, since that is used to react to assets.

begelundmuller · 2024-04-09T14:19:38Z

proto/rill/runtime/v1/api.proto

+message MakeDirRequest {
+  string instance_id = 1 [(validate.rules).string = {pattern: "^[_\\-a-zA-Z0-9]+$"}];
+  string path = 2 [(validate.rules).string.min_len = 1];
+  // Create indicates whether to create the directory if it doesn't already exist
+  bool create = 3;
+  // Will cause the operation to fail if the directory already exists.
+  // It should only be set when create = true.
+  bool create_only = 4;
+}


create and create_only don't really make sense here. Maybe add ignore_if_exists if it's needed?

begelundmuller · 2024-04-09T14:20:33Z

proto/rill/runtime/v1/api.proto

+  rpc MakeDir(MakeDirRequest) returns (MakeDirResponse) {
+    option (google.api.http) = {
+      post: "/v1/instances/{instance_id}/files/make-dir/-/{path=**}",
+      body: "*"
+    };
+  }


Maybe CreateDirectory for consistency with other API names?

begelundmuller · 2024-04-09T14:22:34Z

proto/rill/runtime/v1/api.proto

+  repeated ListFileEntry files = 1;
+}
+
+message ListFileEntry {


Since this type isn't itself a listing, maybe FileEntry?

begelundmuller · 2024-04-09T14:26:00Z

runtime/compilers/rillv1/parser.go

+	var paths []string
+	for _, file := range files {
+		if file.IsDir {
+			continue
+		}
+		paths = append(paths, file.Path)
+	}


What about adding a skipDirs bool option to ListRecursive instead? It could be useful and would avoid some allocations.

begelundmuller · 2024-04-09T14:27:53Z

runtime/drivers/drivers_test.go

+		//withDuckDB,
 		withFile,
-		withPostgres,
-		withSQLite,
+		//withPostgres,
+		//withSQLite,


Needs to be reverted

begelundmuller · 2024-04-09T14:28:25Z

runtime/drivers/repo.go

+type FileEntry struct {
+	Path  string
+	IsDir bool
+}


Should we call it DirEntry to align with Go's type name for it?

Ya makes sense

begelundmuller · 2024-04-10T12:01:53Z

proto/rill/runtime/v1/api.proto


+  rpc CreateDirectory(CreateDirectoryRequest) returns (CreateDirectoryResponse) {


Add a docstring :)

begelundmuller · 2024-04-10T12:02:57Z

proto/rill/runtime/v1/api.proto

@@ -78,6 +78,13 @@ service RuntimeService {
    };
  }

+  rpc CreateDirectory(CreateDirectoryRequest) returns (CreateDirectoryResponse) {
+    option (google.api.http) = {
+      post: "/v1/instances/{instance_id}/files/make-dir/-/{path=**}",


make-dir probably needs to be changed

begelundmuller · 2024-04-10T12:03:54Z

proto/rill/runtime/v1/api.proto

@@ -448,6 +460,15 @@ message PutFileResponse {
  string file_path = 1; // TODO: Redundant, should be removed (but frontend currently uses it)
 }

+message CreateDirectoryRequest {


begelundmuller · 2024-04-10T12:03:58Z

proto/rill/runtime/v1/api.proto

+  bool ignore_if_exists = 3;
+}
+
+message CreateDirectoryResponse {}


begelundmuller · 2024-04-10T12:05:08Z

runtime/compilers/rillv1/parser.go

+	var paths []string
+	for _, file := range files {
+		paths = append(paths, file.Path)
+	}


Should be removed and files renamed to paths (look at the diff vs. main)

begelundmuller · 2024-04-10T12:05:13Z

runtime/compilers/rillv1/parser.go

+	var paths []string
+	for _, file := range files {
+		paths = append(paths, file.Path)
+	}


Same comment

begelundmuller · 2024-04-10T12:07:16Z

runtime/repos.go

+	}
+	defer release()
+
+	// TODO: Handle ignoreIfExists


Can we fix this todo (or remove the ignoreIfExists option entirely)?

Ya i think it makes sense to just remove it. Let it error if the folder exists.

begelundmuller · 2024-04-10T12:08:23Z

runtime/server/repos.go

@@ -62,14 +70,14 @@ func (s *Server) WatchFiles(req *runtimev1.WatchFilesRequest, ss runtimev1.Runti
 	defer release()

 	if req.Replay {
-		paths, err := repo.ListRecursive(ss.Context(), "**")
+		files, err := repo.ListRecursive(ss.Context(), "**", true)


Won't you need to be told about new/deleted directories in WatchFiles?

I have not updated WatchFiles to include folders just yet. I plan to add it in a follow up.

begelundmuller · 2024-04-10T12:08:59Z

proto/rill/runtime/v1/api.proto

+  repeated FileEntry files = 1;
+}
+
+message FileEntry {


Add a docstring

If we're calling it DirEntry internally now, we should probably also call it that here to align names

AdityaHegde added 2 commits April 9, 2024 15:44

Update endpoint to support directories

c7f3740

Update UI

1746169

AdityaHegde requested a review from begelundmuller April 9, 2024 10:27

AdityaHegde self-assigned this Apr 9, 2024

Fix lint

1c4af47

AdityaHegde mentioned this pull request Apr 9, 2024

Rill Developer: A flexible file explorer #4548

Merged

begelundmuller requested changes Apr 9, 2024

View reviewed changes

AdityaHegde added 2 commits April 9, 2024 20:58

PR comments

54607ff

Merge branch 'main' into adityahegde/asset-explorer-list-folders

8fcaebc

begelundmuller requested changes Apr 10, 2024

View reviewed changes

PR comments

56d83e8

AdityaHegde force-pushed the adityahegde/asset-explorer-list-folders branch from 30eda43 to 56d83e8 Compare April 10, 2024 13:24

begelundmuller approved these changes Apr 10, 2024

View reviewed changes

begelundmuller merged commit cc1dbba into main Apr 10, 2024
7 checks passed

begelundmuller deleted the adityahegde/asset-explorer-list-folders branch April 10, 2024 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update ListFiles to return directories and add MakeDir endpoint #4552

Update ListFiles to return directories and add MakeDir endpoint #4552

AdityaHegde commented Apr 9, 2024

begelundmuller Apr 9, 2024

begelundmuller Apr 9, 2024

begelundmuller Apr 9, 2024

begelundmuller Apr 9, 2024

begelundmuller Apr 9, 2024

begelundmuller Apr 9, 2024

AdityaHegde Apr 9, 2024

begelundmuller Apr 10, 2024

begelundmuller Apr 10, 2024

begelundmuller Apr 10, 2024

begelundmuller Apr 10, 2024

begelundmuller Apr 10, 2024

begelundmuller Apr 10, 2024

begelundmuller Apr 10, 2024

AdityaHegde Apr 10, 2024

begelundmuller Apr 10, 2024

AdityaHegde Apr 10, 2024

begelundmuller Apr 10, 2024


		rpc CreateDirectory(CreateDirectoryRequest) returns (CreateDirectoryResponse) {

Update ListFiles to return directories and add MakeDir endpoint #4552

Update ListFiles to return directories and add MakeDir endpoint #4552

Conversation

AdityaHegde commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment