-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add extensibility and various extensions. #88
base: master
Are you sure you want to change the base?
Conversation
- liveness_probe - rate_limiter - storage_allowance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great ideas there! And the code LGTM aside from a few comments and typo fixes. (Haven't tried out the actual features yet.)
@raucao, @DougReeder, et al., thanks for going through this, will get to the comments on priority. What application, if any, should I link to from the "lack of capacity" user feedback page? So they can go to it and clean up files/folders? |
There's no server page (at this point). The user would need to go through all their RS apps. So, we probably need a feature request for a server page which shows the user their largest folders and items. But first, we need the page to show the user what RS apps they've given permission to! |
I was thinking about this as well, and since RS is mostly for programmatically accessing someone's storage, similar to how apps do it on a modern phone OS, most users should not mess around directly with a file inspector (like Inspektor), in my opinion. A server could offer to delete all files for categories that no app is associated with anymore for example. So we could ask the user to check the list of connected apps and remove the ones they don't use/need anymore. And then either ask to delete files directly in that process, or have a cleanup button somewhere.
Since this is pretty much how you would do it on your local hard drive, I think that would be the preferred solution. And it can show which apps are associated with that data, so you can open the apps and selectively delete large items directly from the app interface. PrioritizationUser-friendly solutions aside for a moment, since RS still doesn't support resumable uploads, the files a typical user currently stores are usually not amounting to that much data. An anecdotal example: I use RS apps on a daily basis for the last 8 years at least, including uploading a lot of files via Sharesome, but since none of them are even close to something like raw, full-resolution photos e.g., my main account is still under 1 GB worth of data. However, this would change dramatically, once we have resumable uploads, and I can finally upload my photo collection and create sharable image galleries with thumbnails and such, of course. |
That's a good point - RS items don't necessarily have anything corresponding to a file name. |
Co-authored-by: Râu Cao <sebastian@kip.pe>
Co-authored-by: Râu Cao <sebastian@kip.pe>
Co-authored-by: Râu Cao <sebastian@kip.pe>
@raucao, @DougReeder, got your point: essentially they could willy nilly delete the wrong file and cause their apps to stop working. So we don't want to link inspektor? I can leave the page as is for now, just informative. It's still somewhat actionable. It's a nice intuitive browser. Doesn't show aggregated folder sizes but does show item sizes. |
[removed]
|
Please let's not add UI features (or any other features) to this PR. That should be a separate one, so the people working on front-end design can review and collaborate on it. |
OK, will undo and put in separate PR. Apologies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very promising approach! Taking ideas from Express is a good approach.
We will need to carefully examine the new dispatch sequence, to ensure it is robust, debuggable, and works with streaming, when we implement that. In particular, we'll need to be clear on how errors are passed through middleware. (It's true the Armadietto error handling is not currently systematic.) I have not yet had time to analyze all this.
[2] document rate limiter a bit better [3] add rate limiter tests
…al consumption and error out based on that. The file system size scan is still done only once, on authorization.
I've looked at the new dispatch sequence, and I'm having trouble developing confidence that we've analyzed this enough to be sure this is the way to go. The existing dispatch sequence could use some cleanup, and working with a fixed interface will complicate that. Testing will also be more complicated - we ought to have tests for every combination and order of extensions. If we had a larger, more active pool of developers, these would be reasonable tasks - but we don't. In short, your work is good, but I don't think the team is currently up to supporting it. :-( I worry that the pool of Armadietto-specific extensions will be large enough to be a challenge to support, but too small to cover all use cases. My current thinking is that the best route for a modular RS server in Node is to use a framework like Express, and rework the RS code from Armadietto into middleware. I believe that would allow a very broad range of plugins, and only RS-specific code would need to be maintained by RS developers. |
I'd like to see storage allowance and rate limiter in Armadietto. I think it would be fine to implement them as features controlled by configuration. |
To me this seems like a rather straight-forward thing to add when it's necessary. If it's already better than what's there, and adds two features that we want to have anyway, then why not accept the approach for now, and specify testing requirements for new extensions if and when PRs are opened for them? |
Refactor
lib/armadietto.js
and others to allow extensibility. Idea is to inject middleware extensions similarly to storage providers.Added
threetwo extensions:- liveness probe(edit: not needed as per PR suggestion, using public served files to this end going forward)README.md change has more details.