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

feat!: Align Support for Storage Emulation #2092

Open
danielbankhead opened this issue Oct 21, 2022 · 8 comments
Open

feat!: Align Support for Storage Emulation #2092

danielbankhead opened this issue Oct 21, 2022 · 8 comments
Labels
api: storage Issues related to the googleapis/nodejs-storage API. Breaking Change next major: breaking change this is a change that we should wait to bundle into the next major version priority: p3 Desirable enhancement or fix. May not be included in next release. semver: major Hint for users that this is an API breaking change. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@danielbankhead
Copy link
Contributor

danielbankhead commented Oct 21, 2022

Today, baseUrl is equivalent to the STORAGE_EMULATOR_HOST, when available. This leads to limitations when using the client. Here's the relevant code snippet:

let apiEndpoint = 'https://storage.googleapis.com';
let customEndpoint = false;
// Note: EMULATOR_HOST is an experimental configuration variable. Use apiEndpoint instead.
const EMULATOR_HOST = process.env.STORAGE_EMULATOR_HOST;
if (typeof EMULATOR_HOST === 'string') {
apiEndpoint = Storage.sanitizeEndpoint(EMULATOR_HOST);
customEndpoint = true;
}
if (options.apiEndpoint) {
apiEndpoint = Storage.sanitizeEndpoint(options.apiEndpoint);
customEndpoint = true;
}
options = Object.assign({}, options, {apiEndpoint});
// Note: EMULATOR_HOST is an experimental configuration variable. Use apiEndpoint instead.
const baseUrl = EMULATOR_HOST || `${options.apiEndpoint}/storage/v1`;

Ideally, STORAGE_EMULATOR_HOST should only be a host (without any path associations, e.g. http://localhost:9199) while the client calls the appropriate path on the host (e.g. /storage/v1). That implementation should look something like this: c75b8b8

This requires coordination with Firebase and updates to Firebase Storage Emulator. Additionally, we should explore practices to ensure reliability and a consistent experience between Firebase and Google Cloud Storage (e.g. code consolidation and integration testing).

Related:

Background:

@danielbankhead danielbankhead added Breaking Change type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. semver: major Hint for users that this is an API breaking change. priority: p3 Desirable enhancement or fix. May not be included in next release. next major: breaking change this is a change that we should wait to bundle into the next major version labels Oct 21, 2022
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/nodejs-storage API. label Oct 21, 2022
@danielbankhead danielbankhead self-assigned this Oct 21, 2022
@mgabeler-lee-6rs
Copy link
Contributor

The Go GCS client looks like it had some similar issues in the past. There's a fair bit in that repo if you search issues for STORAGE_EMULATOR_HOST, but this ticket is the closest to this and has two PRs linked to fix it: googleapis/google-cloud-go#2476

The changes there look functionally equivalent to what I proposed in #2070, so I'm wondering if the firebase emulator may already have problems with the Go client? I have no experience with the firebase emulator, so I couldn't say for sure.

A thought: as a stepping stone to avoid a "hard break", might it be useful to temporarily permit a second environment variable indicating how to interpret STORAGE_EMULATOR_HOST, with the initial default being the current behavior, and some specific value indicating to use the "new style", and at some point that default changes, and later that second env var is dropped?

@mgabeler-lee-6rs
Copy link
Contributor

Friendly check-in to see if there's any progress on decisions for how this might move forwards?

@danielbankhead
Copy link
Contributor Author

@mgabeler-lee-6rs hey, thanks for the inquiry; we plan to sync with peer teams on this and other issues over the next few months.

@mgabeler-lee-6rs
Copy link
Contributor

Just checking in on status and confirming that this is still of interest.

@danielbankhead
Copy link
Contributor Author

@mgabeler-lee-6rs We have alignment between teams and we're likely to resolve this in the current quarter.

@danielbankhead danielbankhead changed the title feat: Align Support for Storage Emulation feat!: Align Support for Storage Emulation Jun 22, 2023
@mgabeler-lee-6rs
Copy link
Contributor

Obviously missed Q2, Q3 maybe? :)

@danielbankhead
Copy link
Contributor Author

@mgabeler-lee-6rs yea, I’m aiming for this half :)

@danielbankhead
Copy link
Contributor Author

Update: we had a few other high priority features to take care of; however we still have this as a key item to get to shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/nodejs-storage API. Breaking Change next major: breaking change this is a change that we should wait to bundle into the next major version priority: p3 Desirable enhancement or fix. May not be included in next release. semver: major Hint for users that this is an API breaking change. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

2 participants