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

Set 3000 as the default web port #1215

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Conversation

alagos
Copy link
Contributor

@alagos alagos commented Sep 30, 2024

Having by default PORT= only assigns to that variable 0, which is interpreted by puma to start the web app in a random port when bin/dev is called.
Also exporting the port in this file is useless, as in the foreman context, it's eventually overriden by the settings of your default .env file that has just been copied from .env.example.

@alagos
Copy link
Contributor Author

alagos commented Sep 30, 2024

Worth to mention, I've just started playing with this repo following the setup guide and I faced with the issue where the app was running in a random port every time that I started it (it took me a while to figure out that was because I didn't set the PORT value in the .env file).
For a newcomer as me, will be easier to simply run the app in 3000 instead of trying to figure out where to set this by default (the reason of this PR).

@zachgoll
Copy link
Collaborator

@alagos I think we should leave the bin/dev script as-is so that PORT is an optional variable to set in .env

I agree that given the setup instructions, it could be confusing to include PORT=, so definitely willing to accept that change to PORT=3000

@zachgoll
Copy link
Collaborator

It's worth mentioning—many of the variables in .env.example are 100% optional to get this app running locally.

The goal with that file is to be a comprehensive overview of all the variables that can be used.

We may just need to specify this a bit more clearly in the dev setup guides as I can see where this would be confusing.

Having by default `PORT=` only assigns to that variable `0`, which is
interpreted by puma to start the web app in a random port when `bin/dev`
is called.
@alagos
Copy link
Contributor Author

alagos commented Oct 1, 2024

we should leave the bin/dev script as-is so that PORT is an optional variable to set in .env

That's the thing, it doesn't really matter if you call PORT=8080 bin/dev or you change the file to explicitely use

export PORT="8080"

as this script eventually calls foreman, that completely ignores that variable value, as it's already defined in .env.
Might be nice to have just a single place where to set the port and avoid further confusion, but anyway, I've restored the change for bin/dev.

@zachgoll
Copy link
Collaborator

zachgoll commented Oct 1, 2024

@alagos ah, I see what you're saying now. I agree, the precedence should be command-line arg -> .env -> fallback

So if we have an .env file:

# .env
PORT=5555
  • bin/dev runs on 5555
  • PORT=8888 bin/dev overrides and runs on 8888

@alagos
Copy link
Contributor Author

alagos commented Oct 1, 2024

Not really, having

# .env
PORT=5555

and calling PORT=8888 bin/dev, will always use the port defined in .env, not the command line arg:

12:38:42 web.1    | started with pid 82064
12:38:43 web.1    | To use multipart middleware with Faraday v2.0+, install `faraday-multipart` gem; note: this is used by the ManageGHES client for uploading licenses
12:38:43 web.1    | => Booting Puma
12:38:43 web.1    | => Rails 7.2.1 application starting in development
12:38:43 web.1    | => Run `bin/rails server --help` for more startup options
12:38:43 web.1    | Puma starting in single mode...
12:38:43 web.1    | * Puma version: 6.4.3 (ruby 3.3.4-p94) ("The Eagle of Durango")
12:38:43 web.1    | *  Min threads: 3
12:38:43 web.1    | *  Max threads: 3
12:38:43 web.1    | *  Environment: development
12:38:43 web.1    | *          PID: 82064
12:38:43 web.1    | * Listening on http://0.0.0.0:5555
12:38:43 web.1    | Use Ctrl-C to stop

Same .env, but calling bin/dev

12:46:53 web.1    | started with pid 86848
12:46:54 web.1    | To use multipart middleware with Faraday v2.0+, install `faraday-multipart` gem; note: this is used by the ManageGHES client for uploading licenses
12:46:54 web.1    | => Booting Puma
12:46:54 web.1    | => Rails 7.2.1 application starting in development
12:46:54 web.1    | => Run `bin/rails server --help` for more startup options
12:46:54 web.1    | Puma starting in single mode...
12:46:54 web.1    | * Puma version: 6.4.3 (ruby 3.3.4-p94) ("The Eagle of Durango")
12:46:54 web.1    | *  Min threads: 3
12:46:54 web.1    | *  Max threads: 3
12:46:54 web.1    | *  Environment: development
12:46:54 web.1    | *          PID: 86848
12:46:54 web.1    | * Listening on http://0.0.0.0:5555
12:46:54 web.1    | Use Ctrl-C to stop

won't consider the 3000 fallback defined in bin/dev either (the reason why I previously removed it). Also, using the default copied from .env.example:

# .env
PORT=

and calling PORT=8888 bin/dev, will consider the port defined in .env as 0, so it will start in a random port (and the main cause for this PR existence):

12:41:41 web.1    | started with pid 83808
12:41:41 web.1    | To use multipart middleware with Faraday v2.0+, install `faraday-multipart` gem; note: this is used by the ManageGHES client for uploading licenses
12:41:41 web.1    | => Booting Puma
12:41:41 web.1    | => Rails 7.2.1 application starting in development
12:41:41 web.1    | => Run `bin/rails server --help` for more startup options
12:41:41 web.1    | Puma starting in single mode...
12:41:41 web.1    | * Puma version: 6.4.3 (ruby 3.3.4-p94) ("The Eagle of Durango")
12:41:41 web.1    | *  Min threads: 3
12:41:41 web.1    | *  Max threads: 3
12:41:41 web.1    | *  Environment: development
12:41:41 web.1    | *          PID: 83808
12:41:41 web.1    | * Listening on http://0.0.0.0:51202
12:41:41 web.1    | Use Ctrl-C to stop

@zachgoll
Copy link
Collaborator

zachgoll commented Oct 2, 2024

@alagos I understand that and agree with you. I was commenting on our expected behavior; not the current behavior. I agree that the current behavior is wrong.

@zachgoll zachgoll merged commit 0d7164a into maybe-finance:main Oct 9, 2024
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.

2 participants