stevearc
S3 support via a new adapter
This is still a draft and not completely ready yet, but I might need a bit of help with some of the non-S3 related things (see below). [Update: It is mostly ready now and I think it makes sense to get some feedback before I do much more on it.]
s3 is a bit different from ssh since we don't keep a constant connection and instead make new aws s3 ls calls when moving around, but I have still tried to build similar behavior to what is currently in sshfs.lua, see s3fs.lua.
Having the "filesystem" setup similarly, we then also build the adapter similar to what is done for ssh, see ssh.lua and s3.lua. (Again only with minor differences from the fact that we don't keep an active connection in the same way.)
I am still running into a few problems, among which the primary of the top of my head is:
- I had to temporarily use the scheme
oil-sthree://since any scheme with a number in its name made it so that neovim didn't open the buffer correctly. I am not sure how to best work around this, butoil-s3://would be the clearly better scheme name here. - The mutator is a bit hard to work with from the adapters perspective. It would probably be nicer to move some of its logic to each individual adapter instead.
- There are still a few minor bugs.
- I think there should be a better way to start the Oil s3 view from an already open neovim session, maybe just with
Oil --s3or similar, but I haven't added that yet. - Not really a problem, but more of a note: I haven't updated the README and docs yet. And I have a few
vim.notifycalls to track behavior that should of course be removed before this is ready.
For testing this out I made a new aws account and used extra_s3_args = { "--profile=<my-profile>", "--region=<my-region>" } when running stuff.
If you want to edit some of this yourself if that is easier, feel free to edit directly on this branch. (Also let me know if you want cleaner commits, then I can just rebase.)
This PR will close #633.
Did a short initial review. Overall looks pretty good! Left some inline comments, and a few high level thoughts:
- Any ideas on how to best handle regions? It feels like it might make the most sense as part of the URL
It will probably be hard to find a really good solution for regions. When I wrote that I tested with extra_s3_args = { "--profile=<my-profile>", "--region=<my-region>" } the region part was only necessary for creating buckets, and it was only needed because it is not my default aws user account. For the common use case (i.e., just using your default account and creating buckets in your usual region) everything should work fine, but for other use cases you might need to add the --region flag.
The problem with having the region in the url is that it seems hard to get the region for each bucket. At least I don't know a better solution than something like this
aws s3api get-bucket-location --bucket danielkonge-new-test-bucket --profile=danielkonge
for each bucket (which can be very slow). We could write something for specifying the region when creating buckets though we just wouldn't display it then. I think something like
s3+us-east-1://my_bucket_name/
would be fine for writing when creating buckets, but I would have liked to have this displayed too then.
- Adding the new
buckettype makes me a bit nervous. There are a lot of locations in the codebase that switch and do things with the type. I'd have to do a lot of reading to convince myself that this is safe. If it's possible and not too janky to implement this without adding a new entry type, that would be preferred.
It is probably possible, but I think the new type is the easiest solution I could see. At least I would like to keep something like the changes I have made to view.lua, actions.lua and util.lua, where we display the buckets slightly different than folders. It also makes it easier to match on the entry type in the adapter itself, but that part is not too hard to work around. How would you add the special casing in these files, if I mark a bucket as a "directory"? (See also my comment at the end.)
- If you're looking for a shorter url scheme while waiting for the upstream bug to be fixed, you could use
oil-sss://
I think oil-sss:// is better than oil-three://, so I changed to that for now. When it is possible oil-s3:// would of course still be best.
- I expect that there will be desire in the future to support other forms of blob storage (azure, google cloud, etc). No action items here, I think that with the current layering approach to the API it's already in a decent place for extension, but just something to keep in the back of your mind.
I think the current layout with the "filesystem" (s3fs.lua) and adapter implementation (s3.lua) is pretty easy to work with, so it shouldn't be too difficult to support more adapters like that. It is true that this might make some of the special casing a bit worse though.
For both this and for the entry_type = "bucket" comment, I think it might be easier if some of the code needed in the files I mentioned above is moved into the adapter itself. Then it might be easier to handle the cases correctly in each adapter.
If the region is only really needed for creating new buckets, then maybe we should just not allow the creation of buckets using oil. It seems like a pretty uncommon use case; I would imagine that most of the time this would be used for exploring and manipulating buckets that already exist.
If we also give up the special icon/highlight for buckets then I don't think we would need the new entry type at all. I know it's a slight downgrade in usability, but it keeps all the functionality and significantly decreases the complexity of this change. We might come up with some other way to customize the icons/highlights in the future, since this is something that people keep asking for.
If the region is only really needed for creating new buckets, then maybe we should just not allow the creation of buckets using oil. It seems like a pretty uncommon use case; I would imagine that most of the time this would be used for exploring and manipulating buckets that already exist.
I think I might have confused a bit with what I wrote earlier. The region is generally not needed, and you can create buckets without the region. The only time you might run into problems is when you manually switch aws account with the --profile flag, but this is already an uncommon use case mostly for more advanced users, and they should then also be able to figure out how to add the region correctly. At most I think we might want to mention this in the docs, but I am not even sure that is worth it for such an edge case.
If we also give up the special icon/highlight for buckets then I don't think we would need the new entry type at all. I know it's a slight downgrade in usability, but it keeps all the functionality and significantly decreases the complexity of this change. We might come up with some other way to customize the icons/highlights in the future, since this is something that people keep asking for.
I have removed the entry type and icon/highlight for buckets now, and everything should still work fine.
I also updated the README and docs now.
I fixed the wrong version check and added a quick check of whether aws is executable. Is this what you had in mind?