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

Add condition to change value of port when options.port is specified. #5531

Merged
merged 4 commits into from
Feb 23, 2023

Conversation

aalej
Copy link
Contributor

@aalej aalej commented Feb 17, 2023

Description

Fixes #5521.

The port value is set to 5000 here, but is never changed anywhere else even if the options.port value is specified.

Steps to reproduce issue:

  1. firebase init > functions > any existing project > typescript
  2. Enable helloWorld in index.ts
  3. Run cd functions
  4. Run npm run build
  5. Run firebase functions:shell --port 5005

Output for firebase-tools versions

firebase-tools v11.23.1, v11.18.0, and v11.14.2

http function initialized (http://127.0.0.1:5000/<project_id>/us-central1/helloWorld)

firebase-tools v11.14.1

http function initialized (http://localhost:5005/<project_id>/us-central1/helloWorld)

Scenarios Tested

Checking the value of options.port to be valid, then if the value is a number, change the value of port to the value of options.port. This should now use the passed --port flag value.

  1. firebase init > functions > any existing project > typescript
  2. Enable helloWorld in index.ts
  3. Run cd functions
  4. Run npm run build
  5. Run firebase functions:shell --port 5005
http function initialized (http://127.0.0.1:5005/<project_id>/us-central1/helloWorld).

Sample Commands

firebase functions:shell --port <port>

@yuchenshi yuchenshi enabled auto-merge (squash) February 21, 2023 19:35
@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2023

Codecov Report

Base: 56.12% // Head: 56.12% // No change to project coverage 👍

Coverage data is based on head (8b1d2e6) compared to base (c05b9cd).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5531   +/-   ##
=======================================
  Coverage   56.12%   56.12%           
=======================================
  Files         317      317           
  Lines       21508    21508           
  Branches     4390     4390           
=======================================
  Hits        12072    12072           
  Misses       8374     8374           
  Partials     1062     1062           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -49,6 +49,10 @@ export const actionFunction = async (options: Options) => {
// If the port was not set by the --port flag or determined from 'firebase.json', just scan
// up from 5000
let port = 5000;
if (typeof options.port === "number") {
port = options.port;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that in this implementation, if the specified flag is taken the CLI will gracefully use another port.

Still thinking whether that is the desired behavior or whether the functions:shell command should just error out if the specified port is available.

@aalej - what do think is the right behavior here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @taeold, let me know if I may have misunderstood anything. The current implementation is that when a port is taken, the CLI will try to find an available open port.

If I run firebase functions:shell --port 5005 and port 5005 is taken, the CLI will try to use port 5006 instead - if 5006 is open. I noticed that this behavior is also the same when the port specified in the firebase.json file is taken. E.g. if firebase.json contains the ff:

 "emulators": {
   "functions": {
     "port": 5001
   },
   "ui": {
     "enabled": true
   },
   "singleProjectMode": true
 }

Running firebase functions:shell when port 5001 is taken, the CLI will scan up from 5001 to find an open port.

I’m not sure what would be the best behavior here since currently, the PROS I see with keeping the current implementation is that we don’t have to manually find an open port to connect the emulator to, as the CLI automatically finds it for us. The CLI also provides a warning message similar to below if the specified port is taken(a similar message is also shown when running firebase functions:shell --port <port_number> when the specified port is taken):

⚠  functions: Functions Emulator unable to start on port 5001, starting on 5002 instead.

However, the CONS I see is that if we automatically find an open port, it might cause an issue if some service expects functions to run on the port the user specified. I do agree that in this scenario, it would be better to error out and show a message stating that the specified port is unavailable.

It might be better if we could provide an option for the user to choose if they want to use an open port if the specified port is unavailable. E.g. if the specified port is taken, output a message similar to:

Functions Emulator unable to start on port <specified_port>, would you like to use <open_port> instead? (Y/n)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go ahead with this fix and see what our users think!

@yuchenshi yuchenshi merged commit 07cc578 into firebase:master Feb 23, 2023
@aalej aalej deleted the functions-shell-port branch May 19, 2023 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firebase Functions Shell does not reflect the port number set by specifying the port from the command line.
4 participants