Decouple s3 #688
Decouple s3 #688
Conversation
| options.storage.adapterType && | ||
| !options.storage.adapter | ||
| ) { | ||
| switch (options.storage.adapterType) { |
There was a problem hiding this comment.
I think we could simply check for options.s3 here without the need for an adapterType.
In the case that s3 is not avail, you will need to declaratively pass an adapter and options and rely on the new API. What you think?
There was a problem hiding this comment.
My main reason for using adapterType was the the user would have to specify the following:
adapter: require('oc/src/domain/registry/s3')
It just didn't seem elegant to have people reach down into the source to pull back config info. With that I thought about removing the adapter from the config, but I also liked how I could add my own if I had a proprietary internal datastore that I didn't want to have in open source land.
There was a problem hiding this comment.
Ok, I see what you trying to do.
So, if we are using adapterType only for the default adapter (s3) to avoid people to reach down to get the default adapter we could just make it optional..
- if you don't specify an adapter it will use the s3 one that come with oc
- else you need to declare your own adapter.
This way we don't add an option that is actually only been used only for the default (s3), and we simply sanitize the default adapter. What you recon?
| !options.storage.adapter | ||
| ) { | ||
| switch (options.storage.adapterType) { | ||
| case 's3': |
There was a problem hiding this comment.
As mentioned above this will avoid to have a switch statement on the adapterType, and just setup a default in case for s3 config (legacy) overridable with the new API.
src/registry/domain/repository.js
Outdated
| const options = !conf.local && conf.storage.options; | ||
| const repositorySource = conf.local | ||
| ? 'local repository' | ||
| : conf.storage.adapterType + ' cdn'; |
There was a problem hiding this comment.
Perhaps here we could have the adapter exposing his own name instead ?
maybe something like conf.storage.adapter.type?
There was a problem hiding this comment.
Funny I had it that way first..., but as stated before I wasn't a fan of
adapter: require('oc/src/domain/registry/s3')
There was a problem hiding this comment.
I agree with you and I think we can avoid the user to set adapter: require('oc/src/domain/registry/s3') as that will be the default, so you'll only pass an adapter if you want to override that.
| const settings = require('../../resources/settings'); | ||
| const strings = require('../../resources'); | ||
| const validator = require('./validators'); | ||
| const versionHandler = require('./version-handler'); | ||
|
|
||
| module.exports = function(conf) { | ||
| const cdn = !conf.local && new S3(conf); | ||
| const repositorySource = conf.local ? 'local repository' : 's3 cdn'; | ||
| const cdn = !conf.local && new conf.storage.adapter(conf.storage.options); |
There was a problem hiding this comment.
Nice I like that now we can just pass the options to its own adapter 👍
There was a problem hiding this comment.
This is awesome! Thank you @kmcrawford 🎉🎉🎉
@kmcrawford if it ain't too much of a last request could you remove the package-lock.json from the PR, we are still thinking about adding those...
I'm going to approve this, @matteofigus as I'll be traveling tomorrow could you please merge & publish this?
Next steps:
- we did set-up a monorepo to host all the future work on storage-adapters here: https://github.com/opencomponents/storage-adapters
- we could move the s3 storage adapted there and simply add it as a dependency here,
- as well as starting adding there other adapters as the google-cloud-storage
No description provided.