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.

Sign up for GitHub

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

Jump to bottom

remote filesystems support #203

Closed
wants to merge 1 commit into from
Closed

remote filesystems support #203

wants to merge 1 commit into from

Conversation

marcellourbani
Copy link

Hi Alessandro,
I noticed this extension doesn't work work for remote filesystems.
Since I do like it and mostly work with remotes, I quickly added them.

Worked on 9.30 as the core repo is not currently public, and didn't worry too much about cleanup: I got my deliverable, I'm happy for now.

If you're interested I could port it with more attention to quality to the official release once you shared it

Best regards
Marcello Urbani

@alefragnani
Copy link
Owner

alefragnani commented Apr 19, 2019 •
edited
Loading

Hi @marcellourbani ,

First of all, sorry for taking so long to answer you. I’m a bit busy right now and not able to take the proper attention to my projects. It would take at least another week before coming back.

I don’t use remote file systems, so I didn’t missed that. If I remember correctly, it’s mostly a Linux/MacOS feature, so I’m not sure how to test that in Windows (the only OS I have available).

Looking at the PR, at first, I see no big issue. But I would like to test that before merging. I’ll try to find a way to test 👍 .

I have one question about the PR: what about backward compatibility? I mean, does the new model correctly loads the saved bookmarks (previous session)?

Thanks

@alefragnani alefragnani self-assigned this Apr 19, 2019
@marcellourbani
Copy link
Author

Hi Alessandro,
Remote FS has nothing to do with the OS, I regularly use it from windows (writing my own extension using that feature)

Sadly mine is only for SAP systems, but there are others which use FTP servers, like this which you can try with these settings to access files on a public server (read only I think but that's good enough):

  "remotefs.remote": {
    "local": {
      "scheme": "sftp",
      "host": "test.rebex.net",
      "port": 22,
      "username": "demo",
      "password": "password"
    }
  }

image

I've been using this regularly for a while, and it's generally working, but once in a while I notice bookmark disappearing and didn't find out why yet. Might be linked to file access issues: remote files might be temporarly unavailable. Or might be me playing fast and loose with workspaces (I mostly work as extension developer, so I continually start with a new one)

@marcellourbani
Copy link
Author

Alessandro,
I just found out the bug about losing bookmarks is not related with my changes, happens when saving with the original extension too.

I created issue #207 for that
I think it's safe enough to merge this if you feel like

@marcellourbani
Copy link
Author

@alefragnani
Sorry for not replying your question until now, must have been distracted by finding a working remote for you.

what about background compatibility? I mean, does the new model correctly loads the saved bookmarks (previous session)?

I guess you mean backwards compatibility.
At the best of my knowledge it does. I did my best and in my experience has been working almost flawlessly, but I didn't do a full regression test.

Basically the only significant changes are replacing code like:

if (!fs.existsSync(this.path)) {
     resolve(undefined);
     return;
}

with stuff like:

if ( (this.scheme === "file" || !this.scheme) && !fs.existsSync(this.path)) {
     resolve(undefined);
     return;
}

... which basically removes the fs check for remote files (if the file is on a ftp server fs.existsSync won't work)
The rest of the changes are required to carry around the scheme. The !this.scheme bit is supposed to preserve old bookmarks saved without a scheme, which will always be for a real file

Just let me know if you plan to merge this or should just maintain my fork (starting with fixing #118 :) )

@alefragnani
Copy link
Owner

Sorry for taking so long to answer you too. I'm recovering from a surgery, and were forced to take a break 😄 .

The backward compatibility is exactly what you guess. Thanks for confirming this.

I would like to return to this PR next week, and release a new version right after that.

Thank you

@marcellourbani
Copy link
Author

Ok great thanks
Get well soon!

@alefragnani
Copy link
Owner

Hi @marcellourbani ,

In the recent releases, the VSCode team released newer APIs specially designed to work with remote scenarios, and based in what I could see in documentation and samples, it will probably fit with your scenario too. They released an internal fs alternative, which automatically handles remotes .

I hope that, combining your PR with the newest APIs, would make the update easier 😉

Ref #230

Thank you!

@alefragnani alefragnani added the remote Remote Development set up label Nov 25, 2019
@marcellourbani
Copy link
Author

Hi @alefragnani
I don't know about those, will definitely have a look as soon as I find the time and come back to you

Marcello

@marcellourbani
Copy link
Author

@alefragnani I just tested the latest version, still doesn't work with remote filesystems.

PS: I will probably end up publishing a fork so I don't have to reinstall manually at every sync. Don't care about monetising or taking credit

@alefragnani
Copy link
Owner

Yes @marcellourbani , the latest update still doesn’t have this, sorry. It was originally planned, but I had a few issues while testing some scenarios, and decided to hold a bit. I would like to resume the work next week, and release an update sometime in April.

have to reinstall manually at every sync

What do you mean about that? if you use your own VSIX file, it shouldn’t show updates on VSCode.

@marcellourbani
Copy link
Author

marcellourbani commented Mar 31, 2020 via email

@alefragnani
Copy link
Owner

Do you about ignoreExtensions setting in Settings Sync? It says you can which extensions it should ignore while syncing. (I don’t use Settings Sync, so I’m not entirely sure how it works and if there are any limitations)

@marcellourbani
Copy link
Author

marcellourbani commented Mar 31, 2020 via email

@alefragnani
Copy link
Owner

No, this setting is from Settings Sync extension (not VSCode Insiders 😄

https://github.com/shanalikhan/code-settings-sync#global-settings

@marcellourbani
Copy link
Author

marcellourbani commented Mar 31, 2020 via email

@alefragnani
Copy link
Owner

That's great to see it worked!

And yes, it should, and will, be merged. Thanks 👍

@alefragnani alefragnani added this to the 11.2.0 milestone Apr 26, 2020
@alefragnani alefragnani modified the milestones: 11.2.0, 11.3.0 May 8, 2020
@alefragnani alefragnani modified the milestones: 11.3.0, 11.4.0 Jun 16, 2020
@alefragnani alefragnani modified the milestones: 11.4.0, 12.0.0 Oct 17, 2020
@alefragnani alefragnani modified the milestones: 13.0.0, January 2021 Jan 27, 2021
@alefragnani
Copy link
Owner

Hi @marcellourbani ,

The Remote Support should be finally released, this weekend! And I hope it will work for you...

I didn't use your PR as is, but combined with other ideas I found/discovered while testing some scenarios. Instead of adding uri on each bookmark, as on your PR, I used a relative path approach, similar to some other extensions (like CodeTour) does. Doing so, I was able to ignore the Uri.scheme. So, if I open the same folder locally or remotely, the bookmarks.json file (when saved inside the project) will be the same, and can be accessed/updated on both locations.

The only remote system I used was Docker, but I suppose it would work on SSH, WLS and others as well. I would be very happy to know it works for you too.

Beforehand, you could install/test my other Numbered Bookmarks extension, which was updated yesterday with the same feature. If it works for you, the Bookmarks extension should work as well.

⚠️ When the extension is updated, the bookmarks.json file (or in memory) format is automatically updated. If you revert the version, you will lose the bookmarks. So, I suggest you to test on a single folder/project first, and if everything works as expected, enjoy 😄 . Otherwise, rollback to your original build, and let me know that happened.

Thanks for your contribution, and patience

@marcellourbani
Copy link
Author

I can confirm, numbered bookmarks works fine for my use case
Thanks a lot

@alefragnani
Copy link
Owner

That's great to hear!

Stay tuned for the updated release of the Bookmarks extension this weekend 😬

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alefragnani alefragnani

Labels
enhancement remote Remote Development set up
Projects
None yet
Milestone
February 2021
Development

Successfully merging this pull request may close these issues.
2 participants
@marcellourbani @alefragnani