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 PORT environment variable being ignored #280

Merged
merged 1 commit into from
Jan 2, 2023

Conversation

porst17
Copy link
Contributor

@porst17 porst17 commented Dec 10, 2020

I noticed that bin/reload does not respect the PORT environment variable contradicting

$ ./bin/reload --help | grep -A 2 -- --port
  -p, --port [port]              The port to bind to. Can be set with PORT env
                                 variable as well. Defaults to 8080. (default:
                                 "8080")

This provides a quick fix for this.

If provided, --port will take precedence.

@alallier
Copy link
Owner

alallier commented Jun 9, 2021

Thanks for the PR and sorry for the long reply. Command line arguments are meant to task precedence over environment variables. With that being said I'm going to close this

(Light) Reference: https://stackoverflow.com/questions/11077223/what-order-of-reading-configuration-values

@alallier alallier closed this Jun 9, 2021
@porst17
Copy link
Contributor Author

porst17 commented Jun 11, 2021

My patch is respecting the order the StackOverflow post is proposing.

  • If --port [port] is present, port will be used (command line argument)
  • If --port [port] is not present, but the PORT environment variable is, then PORT is used (environment)
  • If PORT is not present, 8080 is used (global configuration)

My point was that the PORT environment variable is never used, even though the documentation says it does.

Your current implementation does the following:

  • If --port [port] is present, port will be used (command line argument)
  • If --port [port] is not present, 8080 is used (global configuration)

The doc says, PORT will be used, but it never is.

Example:

$ PORT=1234 npx reload .
npx: Installierte 52 in 2.739s

Reload web server:
listening on port 8080

The server should listen on port 1234, but instead it listens on 8080.

Please reconsider merging my PR.

@alallier alallier reopened this Aug 24, 2021
@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #280 (8cda46a) into master (39f2dc8) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #280   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          131       131           
=========================================
  Hits           131       131           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39f2dc8...8cda46a. Read the comment docs.

@porst17
Copy link
Contributor Author

porst17 commented Aug 30, 2021

Thanks for reopening. Are there still any objections against merging this?

@alallier alallier merged commit 8eea09e into alallier:master Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants