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

"HOST" environment variable #6423

Merged
merged 2 commits into from
Sep 8, 2023
Merged

"HOST" environment variable #6423

merged 2 commits into from
Sep 8, 2023

Conversation

Cyanoure
Copy link
Contributor

@Cyanoure Cyanoure commented Sep 8, 2023

Hi!
I added the "HOST" environment variable. It works the same way as the "PORT".
I also added tests for it depending on "PORT" tests.

Fixes #6422

Added "HOST" environment variable. It works the same way as the "PORT".
@Cyanoure Cyanoure requested a review from a team as a code owner September 8, 2023 02:49
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #6423 (6c270ea) into main (eb498b0) will increase coverage by 0.02%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6423      +/-   ##
==========================================
+ Coverage   73.61%   73.64%   +0.02%     
==========================================
  Files          31       31              
  Lines        1880     1882       +2     
  Branches      406      407       +1     
==========================================
+ Hits         1384     1386       +2     
  Misses        419      419              
  Partials       77       77              
Files Changed Coverage Δ
src/node/cli.ts 90.90% <100.00%> (+0.06%) ⬆️

Continue to review full report in Codecov by Sentry.

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

Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

Thank you! I love the addition of tests as well.

What do you think about making this CODE_SERVER_HOST instead? I know we already have plain PORT but we are trying to ensure new variables have unique names (and eventually we should add CODE_SERVER_PORT and deprecate PORT too).

@Cyanoure
Copy link
Contributor Author

Cyanoure commented Sep 8, 2023

Thank you! I love the addition of tests as well.

What do you think about making this CODE_SERVER_HOST instead? I know we already have plain PORT but we are trying to ensure new variables have unique names (and eventually we should add CODE_SERVER_PORT and deprecate PORT too).

I think it will be fine.
But for the port, I don't know. I mean, in the past when I tried Heroku, it used PORT. I had to use that environment variable to set the listening port of my nodejs server.
I don't know if anyone uses code-server in an environment like that.

Thanks for the review!

@code-asher
Copy link
Member

Thank you for the input and the examples! Collisions like that are a really good reason we should use a unique prefix.

For this PR let us go forward with CODE_SERVER_HOST, then. No need to make any changes to PORT here (unless you want to), I can do that in another PR.

@code-asher
Copy link
Member

code-asher commented Sep 8, 2023

Oh wait, were you saying that Heroku sets PORT so we actually should keep PORT? I think I misunderstood it as Heroku itself using PORT and that was colliding with code-server.

@Cyanoure
Copy link
Contributor Author

Cyanoure commented Sep 8, 2023

Oh wait, were you saying that Heroku sets PORT so we actually should keep PORT? I think I misunderstood it as Heroku itself using PORT and that was colliding with code-server.

Yes :D
I'm sorry if I was unclear.

Then I'll add a new commit for replacing HOST to CODE_SERVER_HOST.

@Cyanoure Cyanoure requested a review from code-asher September 8, 2023 22:18
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

Thanks again, looks good!

Sounds like we will probably want to keep PORT around then. I will keep that in mind!

@code-asher code-asher enabled auto-merge (squash) September 8, 2023 22:32
@code-asher code-asher merged commit a76e524 into coder:main Sep 8, 2023
10 checks passed
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.

[Feat]: Set the host address with environment variable
2 participants