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

"IDrive" concept for ContentsManager #2248

Merged
merged 13 commits into from May 25, 2017

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 19, 2017

Proposal for multiple "drive" backends to the contents manager. Basic structure:

  1. The ContentsManager has a default Drive backend which talks to the normal filesystem.
  2. It also has the ability to add new IDrives, which can talk to filesystems on other servers.
  3. Drive backends are distinguished via their paths, where paths have a structure given by driveName:path/to/file.
  4. The ContentsManager becomes essentially a routing system, deciding which IDrive to dispatch file operations to.
  5. FileBrowserModels can take an optional driveName argument, which they prepend to the path for file operations, so that a given FileBrowserModel talks to a specific drive (though still through the DocumentManager/ContentsManager).
  6. This will allow for the DocumentManager and ServicesManager to be singletons.
@blink1073
Copy link
Member

@blink1073 blink1073 commented May 22, 2017

Looks good! One thing I would add is a rest endpoint to Drive.IOptions, so we can use it for files and the settings system.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented May 22, 2017

Sure thing. Is there an example pattern somewhere in the codebase to follow?

@blink1073
Copy link
Member

@blink1073 blink1073 commented May 22, 2017

To date all of the rest endpoints are hardcoded constants in the file. We can default to that if the override is not given.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented May 22, 2017

A couple of wrinkles to consider with this approach:

  1. Using the pattern driveName:path/to/file means reserving the ':' character, a restriction not previously in place.
  2. I have chosen to not qualify the default IDrive with a name, so paths using the default filesystem look identical to their previous form. It would be easy to change that to something like default:path/to/file if we wanted.
  3. In this approach, almost every path that the lab application sees is the "global" path, and almost every path that the IDrive sees is the "local" path (though it still leaks through in a couple of places). The downside of this is that the ContentsManager has to do a lot of monkeying with the paths when passing file operations around.

@blink1073
Copy link
Member

@blink1073 blink1073 commented May 22, 2017

I'm happy with the current approach.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented May 22, 2017

Okay, great. Tests and cleanup incoming.

@ian-r-rose ian-r-rose force-pushed the contents_manager_drives branch from e40ac90 to 5a9044a May 23, 2017
@ian-r-rose ian-r-rose changed the title [WIP] "IDrive" concept for ContentsManager "IDrive" concept for ContentsManager May 23, 2017
@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented May 25, 2017

@ian-r-rose can you post a few screen shots showing what this looks like in the various parts of the UI when there are multiple drives loaded?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented May 25, 2017

Here is a screenshot showing an additional filebrowser in the right area:
filebrowsers
I would note that this PR doesn't really address any modifications to the UI/UX we might want to make for multiple filebrowsers: it just allows for path-based disambiguation of different server backends. The simplest thing to support at the moment is another filebrowser tab, but this approach is fairly agnostic to the ultimate UI choices.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented May 25, 2017

@blink1073
Copy link
Member

@blink1073 blink1073 commented May 25, 2017

Is this done?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented May 25, 2017

Yes, should be good to go.

@blink1073
Copy link
Member

@blink1073 blink1073 commented May 25, 2017

w00t

@blink1073 blink1073 merged commit 9b3c05d into jupyterlab:master May 25, 2017
2 checks passed
@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented May 25, 2017

Huzzah!

@ian-r-rose ian-r-rose deleted the contents_manager_drives branch May 25, 2017
@blink1073 blink1073 mentioned this pull request Jun 1, 2017
@itcarroll
Copy link

@itcarroll itcarroll commented Sep 21, 2018

Sorry to wake a sleeper here, but ... how do you actually create that second filebrowser tab with a given "root" (for lack of better word), i.e. the path the home icon in the filebrowser represents? I can't find an existing extension that would let me add another file browser pointed at a mounted volume on the server, and this seems like it provides all that's needed to do that.

I'm new to jupyter extensions, but have worked through the xkcd extension, and think I can do this if pointed in the right direction. Like the code that produced @ian-r-rose's demo above?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Sep 21, 2018

Hi @itcarroll, you may need two components to this: a server extension that creates a new ContentsManager pointed to your mounted volume, and a jupyterlab extension that creates a new IDrive and filebrowser. That IDrive can then point to your new serverextension.

Both the @jupyterlab/github and @jupyterlab/google-drive implement examples of the labextension portion.

@itcarroll
Copy link

@itcarroll itcarroll commented Sep 23, 2018

thanks for the pointers, @ian-r-rose. i got the easy stuff started in itcarroll/jupyterlab-volume. Comments and corrections welcome; fair warning, i'll sling more questions at you over there if you do comment 🤓.

@jasongrout jasongrout removed this from the 1.0 milestone Feb 1, 2019
@jasongrout jasongrout added this to the 0.23 milestone Feb 1, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants