Add S3 storage service

[LordMike]

(do not merge)

What does this PR do?

Implements an S3 storage service

Closes Issue(s)

Fixes #136

Motivation

If we deploy BaGet, we'll need S3 storage support :)

Additional Notes

This code has not been tested, yet. I'd like some feedback before proceeding. It's basically a copy off of the Azure project, with the search removed. I've replicated the configuration validation methods etc.

[LordMike]

@loic-sharma I have a few thoughts and questions.

Shouldn't the storage services be simpler than this?

I would imagine that it would be possible to have a storage service inteface that implements the following:

• Get(string path)
• GetDownloadUrl(string path)
• Put(string path, Stream content, string contentType)
• Delete(string path)

Then a service could be built around this, that exposes the Nuspec/Package/Readme specifics. All storage services would be shielded from new package types (.snupkg), and services could expose new features without breaking implementations.

Adding the GetDownloadUrl() is useful for redirections, see #25. S3 would provide pre-signed urls (or whatever it needs to provie). Azure would do the same.

Is it necessary to add all storage implementations in the web project?

It seems odd to me to add all services, and then finally "remap" one service by adding it as the intended interface. Wouldn't it be better to have the wanted service register itself as the intended interface directly?

Why put the storage clients in the DI framework?

This is more of a preference thing: Why add the Azure Blobs-specific client to the DI, only to pull it out and give it to one specific implementation? It's not like it can be exchanged (no use of interfaces :P) .. so why not create the Azure Blobs client within BlobPackageStorageService (and the AmazonS3Client in S3StorageService for S3)?

[LordMike]

Note: I did register the services in the web project, but didn't commit it because I missed it -- I'm seeing a lot of newline changes which I'm trying to figure out.

[loic-sharma]

@LordMike great suggestions. I went ahead and opened a PR that addresses your concerns: #145. Please let me know what you think! 😄

[LordMike]

@loic-sharma I've updated this implementation for #145, it's been tested and can successfully upload, download packages.

I have not tested the deletes or get-uri, as BaGet does not currently expose those.

[loic-sharma]

Looks good! I left some minor comments, please address them and I'll merge these changes in :)

[LordMike]

@loic-sharma I've just noticed ValidateOptions is never called. Neither is Azure's version.

Where should this be used?

[loic-sharma]

@loic-sharma I've just noticed ValidateOptions is never called. Neither is Azure's version.

Oh whoops, good catch! I improved BaGet's configuration validation in #147. You should now register your configuration using services.ConfigureAndValidate<MyOptions>(...) or services.ConfigureAndValidateSection<MyOptions>(...). This will verify that the configs' values match the option's annotations. For example, the BlobStorageOptions are configured here.

[LordMike]

@loic-sharma updated.

[LordMike]

I also changed a few ConfigureAndValidate<T>() to ConfigureAndValidateSection<T>()

[loic-sharma]

Could you rebase this off of the latest master? It's a little tricky to review right now with the line ending changes. Thanks!

[LordMike]

I wasn't able to rebase, but I did copy all the changes to a new commit.

[loic-sharma]

This is looking good. I left some final feedback, once that's addressed I'll merge this in :)

[loic-sharma]

Thank you @LordMike for the great work! 🎉