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

FR: Support Cloud Storage #71

Open
takuya1981 opened this issue Jun 10, 2019 · 6 comments
Open

FR: Support Cloud Storage #71

takuya1981 opened this issue Jun 10, 2019 · 6 comments
Labels
api: storage help wanted Extra attention is needed type: feature request A new feature API is requested

Comments

@takuya1981
Copy link

firebase-admin-dotnet does not support Cloud Storage yet.
I'd like to contribute for it.

The simple specifications are as follows:

  • Need new class to handle Cloud Storage. Its name is FirebaseAdmin.Cloud.FirebaseStorage as an example.
  • FirebaseStorage instance is managed in FirebaseApp class.
  • FirebaseStorage instance has Google.Cloud.Storage.V1.StorageClient instance.
  • FirebaseStorage instance will return the StorageClient instance associated with the Firebase app.
  • Now the Bucket class is returned in other Java/Python SDK. But the Bucket class in .NET is almost data class. It's not neccesary to be returned(It's my opinion).
  • When calling FirebaseApp.Delete(), StorageClient.Dispose() is called.

I would like to open a pull request about the above if there are no problems.

@hiranya911
Copy link
Contributor

Seems like a good addition. Except I wouldn't call the class FirebaseStorage. Perhaps StorageClientHelper or just GoogleCloudStorage.

using FirebaseAdmin.Cloud;
using Google.Cloud.Storage.V1;

StorageClient client = StorageClientHelper.DefaultInstance.GetClient();

@hiranya911 hiranya911 added api: storage help wanted Extra attention is needed type: feature request A new feature API is requested labels Jun 10, 2019
@takuya1981
Copy link
Author

Name is StorageClientHelper, I understand.

StorageClient support custom encryption key like this.

public static StorageClient Create(GoogleCredential credential = null, EncryptionKey encryptionKey = null);

This class should support this function like this.

public static StorageClient GetStorageClient(EncryptionKey encryptionKey = null){}
public static StorageClient GetStorageClient(FirebaseApp app, EncryptionKey encryptionKey = null)

What do you think?

@hiranya911
Copy link
Contributor

Hi @takuya1981. Thanks again for thinking about this. I'm not convinced that we should expose custom encryption keys at this stage. We haven't exposed it in other languages. Therefore let's start with just GetStorageClient() for now. Note that GetStorageClient(FirebaseApp) won't be necessary if we attach CloudStorageHelper instance to a FirebaseApp. So we are looking at something like:

public class CloudStorageHelper
   public static CloudStorageHelper DefaultInstance;
   public static CloudStorageHelper GetCloudStorageHelper(FirebaseApp app);
   public StorageClient GetStorageClient();

We can support encryption keys in the future if we want to, but that should be supported across all languages if we do that.

@hiranya911
Copy link
Contributor

Correction. We can just expose the StorageClient as a static:

public class CloudStorageHelper
   public static StorageClient GetStorageClient();
   public static StorageClient GetStorageClient(FirebaseApp app);

@takuya1981
Copy link
Author

I understand that encryption key is not needed at the moment and signature of method.

And I opened draft pull request, draft means that encryption key is still supported, I'll delete it.

@takuya1981
Copy link
Author

I opened pull request. Please review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage help wanted Extra attention is needed type: feature request A new feature API is requested
Projects
None yet
Development

No branches or pull requests

2 participants