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

Increases timeOut for detectFromPort to avoid sporadic deployment fails #6888

Closed
wants to merge 2 commits into from

Conversation

takahashi-shotaro
Copy link

Co-authored-by: @himanshu810e

Description

fix #5888 (This staled issue has a lot of comments attached to it, but it has not been re-opened.)

This comment stating that the issue was resolved by extending the timeout has received many reactions.

This PR is based on #5834 with additional comments. #5834 has not been reviewed for a long time (perhaps the reviewer is not active?).

Scenarios Tested

Sample Commands

Co-authored-by: Himanshu Chaudhary <himanshu.cy@gmail.com>
@Matt-Jennings-GitHub
Copy link

Any updates here? Would be great to see this suggestion finally merged 🙏

@takahashi-shotaro
Copy link
Author

takahashi-shotaro commented May 6, 2024

@Matt-Jennings-GitHub The issue corresponding to this PR is closed, so this PR may never be reviewed. If you follow the comments below and create an issue, the maintainer may respond to the issue. (I have not been able to create an Issue because our code is used in production and it is hard to reproduce and report problems.)

#5888 (comment)

@Matt-Jennings-GitHub
Copy link

Thanks @takahashi-shotaro - also in prod. here but will see what I can do

@Oxpinguin
Copy link

Hello, because of the underlying issue many colleagues of mine can not perform deployments and are totally blocked.

Is there any chance to see this integrated? Thanks!

@MNITD
Copy link

MNITD commented May 10, 2024

Hey @takahashi-shotaro
I believe 30 seconds is a good starting point for improvement
However, a better option is to provide a way for users to set the timeout value via some CLI option or env var
What do you think?

UPD

I just tested the solution with a custom bash script that updates the firebase-tools source by explicitly passing the timeout value instead of relying on the default one
At first, I tried 50 seconds and that wasn't enough (I even started thinking that the problem was somewhere else)
But then for the experiment, I changed the timeout to 300 seconds and it passed after ~54 seconds

I believe the more complex code someone will have the more time it might take for Firebase CLI to wait for the server to be ready to return the functions.yaml config file

So changing one hard-coded value onto another is not a solution at all, we should provide a dynamic option instead

@takahashi-shotaro
Copy link
Author

@MNITD I agree. However, most people including those who opened #5888, probably have problems with the timeout being changed from 30 seconds to 10 seconds a year ago. So I simply revert the change.

#5888 (comment)

@takahashi-shotaro
Copy link
Author

takahashi-shotaro commented May 10, 2024

IMO, It would be better to simply report a time elapsed warning instead of forcing a stop at a timeout (at least on the local machine).

@takahashi-shotaro
Copy link
Author

takahashi-shotaro commented May 11, 2024

@MNITD I made another Issue & PR to remove timeout and fix it #7167

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants