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

Add extensibility and various extensions. #88

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

JakubNer
Copy link
Contributor

@JakubNer JakubNer commented Mar 14, 2022

Refactor lib/armadietto.js and others to allow extensibility. Idea is to inject middleware extensions similarly to storage providers.

Added three two extensions:

  • storage allowance
  • rate limiter
    - liveness probe (edit: not needed as per PR suggestion, using public served files to this end going forward)

README.md change has more details.

Jakub Ner added 3 commits March 14, 2022 06:40
Copy link
Member

@raucao raucao left a 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.)

example/index.html Outdated Show resolved Hide resolved
lib/controllers/base.js Show resolved Hide resolved
lib/extensions/liveness_probe/liveness_probe.js Outdated Show resolved Hide resolved
lib/extensions/liveness_probe/liveness_probe.js Outdated Show resolved Hide resolved
spec/armadietto/middleware_spec.js Outdated Show resolved Hide resolved
spec/armadietto/middleware_spec.js Outdated Show resolved Hide resolved
@JakubNer
Copy link
Contributor Author

JakubNer commented Mar 16, 2022

@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?

image

@DougReeder
Copy link
Contributor

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!

@raucao
Copy link
Member

raucao commented Mar 16, 2022

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.

So, we probably need a feature request for a server page which shows the user their largest folders and items.

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.

Prioritization

User-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.

@DougReeder
Copy link
Contributor

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.

That's a good point - RS items don't necessarily have anything corresponding to a file name.

JakubNer and others added 4 commits March 17, 2022 06:32
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>
@JakubNer
Copy link
Contributor Author

we need the page to show the user what RS apps they've given permission to

mostly for programmatically accessing someone's storage, similar to how apps do it on a modern phone OS, most users should not mess around

@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.

@JakubNer
Copy link
Contributor Author

JakubNer commented Mar 19, 2022

[removed]

Added a "Terms of Service" button that shows up iff the terms_of_service_url configuration point is present:

@raucao
Copy link
Member

raucao commented Mar 19, 2022

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.

@JakubNer
Copy link
Contributor Author

JakubNer commented Mar 19, 2022

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.

Copy link
Contributor

@DougReeder DougReeder left a 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.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
spec/armadietto/middleware_spec.js Show resolved Hide resolved
@DougReeder
Copy link
Contributor

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.

@DougReeder
Copy link
Contributor

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.

@raucao
Copy link
Member

raucao commented Aug 16, 2022

Testing will also be more complicated - we ought to have tests for every combination and order of extensions.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants