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 ability to specify server port from cli #2239

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

just-an-engineer
Copy link

It now looks to see if a port number was specified on the command line, and tries to use that before looking at the config file or environment variables
Since --start-server calls InternalStartServer under the hood, I have it starting it with the port number specified as the environment variable, although I can change it to also take it from the cli.
Added tests and updated the readme for usages

@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2024

Codecov Report

Attention: Patch coverage is 32.82443% with 88 lines in your changes missing coverage. Please review.

Project coverage is 51.79%. Comparing base (0cc0c62) to head (f07dc9b).
Report is 72 commits behind head on main.

Files with missing lines Patch % Lines
src/util.rs 27.27% 13 Missing and 43 partials ⚠️
src/commands.rs 36.36% 2 Missing and 12 partials ⚠️
src/config.rs 43.75% 1 Missing and 8 partials ⚠️
src/cmdline.rs 42.85% 0 Missing and 8 partials ⚠️
tests/oauth.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2239       +/-   ##
===========================================
+ Coverage   30.91%   51.79%   +20.87%     
===========================================
  Files          53       54        +1     
  Lines       20112    20550      +438     
  Branches     9755     9540      -215     
===========================================
+ Hits         6217    10643     +4426     
- Misses       7922     7956       +34     
+ Partials     5973     1951     -4022     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sylvestre
Copy link
Collaborator

What about configuring it in a configuration file?

@just-an-engineer
Copy link
Author

Yeah, I can do that. I'll add that as well

@just-an-engineer
Copy link
Author

Alright, just added that in. Currently, this is the order of precedence in the code of where it looks for the port: CLI, config, env var, and lastly, the hardcoded default port of 4226. No particular reason for that order, so that can be changed up if desired.

README.md Show resolved Hide resolved
@just-an-engineer
Copy link
Author

@sylvestre, I'm a bit new to the whole process. At this point, do I select "Update branch"? Do I want for someone else to do something?

@just-an-engineer just-an-engineer marked this pull request as draft August 12, 2024 17:47
just-an-engineer and others added 2 commits August 13, 2024 11:09
… operations (such as a compile request) without having to specify the non-default port every time
@just-an-engineer just-an-engineer marked this pull request as ready for review August 13, 2024 15:11
@just-an-engineer
Copy link
Author

I realized I hadn't done anything for commands outside of start or stop. So I added in basic functionality for a client cache (which we can expand to other things), and added tests for that. I also corrected a prior test that was incorrect, I just wasn't thinking properly for that one. I also expanded it to test that a compile request works for a server started on a non-default port.
The client cache will also help it be more ergonomic for users because they won't have to specify an environment variable alongside every sccache command they run, because if they started a server on a different port, they'd have to use the env vars to see its stats, run a compile request, etc. This can make it a bit more streamlined by assuming that you'll have a single compile server you primarily use (if that's not the case, this may break down, but it's not like it was easier beforehand), and once you specify it, that's the one you want to see the stats for, run compile jobs from, and shutdown

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.

3 participants