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

fix: Improve STORAGE_EMULATOR_HOST usage #2470

Closed
wants to merge 1 commit into from

Conversation

le0pard
Copy link

@le0pard le0pard commented May 16, 2024

Problem

  • if provided STORAGE_EMULATOR_HOST env, apiEndpoint == baseUrl. This lead to invalid url to fetch data, because no storage/v1 in baseUrl
  • if provided apiEndpoint, in this case all works

Expected behaviour

  • STORAGE_EMULATOR_HOST env works same way as apiEndpoint property

Fix

  • Apply same logic for STORAGE_EMULATOR_HOST as for apiEndpoint to generate baseUrl

@le0pard le0pard requested review from a team as code owners May 16, 2024 16:39
Copy link

google-cla bot commented May 16, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. api: storage Issues related to the googleapis/nodejs-storage API. labels May 16, 2024
Copy link

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@le0pard le0pard changed the title Fix usage for STORAGE_EMULATOR_HOST [storage] Improve STORAGE_EMULATOR_HOST usage May 16, 2024
Fix usage for STORAGE_EMULATOR_HOST together with baseUrl
@le0pard le0pard changed the title [storage] Improve STORAGE_EMULATOR_HOST usage fix: Improve STORAGE_EMULATOR_HOST usage May 16, 2024
@ddelgrosso1
Copy link
Contributor

Hi @le0pard we appreciate the contribution. However, there is a reason we do not append pathing to STORAGE_EMULATOR_HOST. For more details see: #2087 and #2092

@le0pard
Copy link
Author

le0pard commented May 16, 2024

Thanks @ddelgrosso1 Should we just introduce new env variable, which will work the same as apiEndpoint, while team busy make STORAGE_EMULATOR_HOST work with Firebase Storage emulator ?

@ddelgrosso1
Copy link
Contributor

@le0pard fair question. Let me check internally with the firebase team as to what the current status is. The problem with adding a new variable is it is one more thing to maintain and deprecating something else becomes a long task.

Is there a problem with using apiEndpoint for your use case?

@le0pard
Copy link
Author

le0pard commented May 16, 2024

Thanks @ddelgrosso1

apiEndpoint is exactly how I resolve this issue for now. Problem, that in project we are using node.js and python code. In python sdk STORAGE_EMULATOR_HOST works as expected ( https://github.com/googleapis/python-storage/blob/main/google/cloud/storage/client.py#L168-L171 ) , but not in node.js sdk. So in python code I dont need create any additional code for sdk to switch endpoint for local development. Just pass env variable in docker container.

In JS codebase I need create separate env variable like GCLOUD_API_ENDPOINT and write code, which will check this env variable to make sdk work in same way, as python:

const {
  Storage: GoogleStorage
} = require('@google-cloud/storage')

const STORAGE_ENDPOINT = process.env.GCLOUD_API_ENDPOINT ?? undefined

const storage = new GoogleStorage({
  apiEndpoint: STORAGE_ENDPOINT
})

And of course I cannot share same STORAGE_EMULATOR_HOST env variable, because it is "break" node.js sdk right now

@le0pard le0pard closed this May 16, 2024
@le0pard le0pard deleted the fix-emulator-host branch May 16, 2024 18:13
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. size: xs Pull request size is extra small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants