server: get default port optionally form environment var

[AvdN]

If the default port 8080 is taken, you have to either change the app.run() or use --port on every invocation of the program. When trying out examples I don't want to do the first and forget the second.

This patch allows you set the environment variables LONA_DEFAULT_HOST and LONA_DEFAULT_PORT in your shell. These override the built-in defaults ('localhost' resp. 8080), and are in turn overridden by arguments to app.run() and startup commandline parameters.

[fscherf]

Really nice! Thanks for this feature. Only a very small change is required to merge it.
Could you add the prefix "server: " to the commit message as well?

[fscherf]

We will need a check here if the value of LONA_DEFAULT_PORT can be type casted to an int. This will crash into a cryptic traceback if not

[AvdN]

had not realised that if you amend the title of the commit, that doesn't percolate through into the PR.
If you do

 LONA_DEFAULT_PORT=abc python example.py

you'll get a message:

value "abc" for environment variable "LONA_DEFAULT_PORT" cannot be converted to an integer port number

after which the system sys.exits with value 2 (same value as when using --port abc, however I realise you cannot have the above and specify --port 8081, as it will still error on the environment variable not converting. This might be circumvented by assigning the environment variable to port and doing the check-for-int-ness after setattr has optionally been updated by the commandline-parsing.

But if you have an incorrect LONA_DEFAULT_PORT setting, IMO that should be fixed, not suppressed by doing --port.

[fscherf]

I did some testing and found out the check is just not necessary (sorry). Argparse does exactly what you would expect and will handle 8080 and "8080". So we just have to add port=env.get('LONA_DEFAULT_PORT', '8080') in these two spots, and everything should be fine.

lona/lona/app.py

Line 432 in 7f8ba4c

port=8080,

lona/lona/command_line/handle_command_line.py

Line 119 in 7f8ba4c

default=8080,

[AvdN]

I did some testing and found out the check is just not necessary (sorry). Argparse does exactly what you would expect and will handle 8080 and "8080". So we just have to add port=env.get('LONA_DEFAULT_PORT', '8080') in these two spots, and everything should be fine.

lona/lona/app.py

Line 432 in 7f8ba4c

port=8080,

lona/lona/command_line/handle_command_line.py

Line 119 in 7f8ba4c

default=8080,

I did this but cannot re-apply for a review, probably because there is already a review request pending.

[fscherf]

@AvdN: Yeah no worries, that's on me.
I think that's it! I will do some more local testing, but the patch should be mergeable in this state

[codecov-commenter]

Codecov Report

Patch coverage has no change and project coverage change: -0.02% ⚠️

Comparison is base (7f8ba4c) 71.81% compared to head (113cfd8) 71.80%.

❗ Current head 113cfd8 differs from pull request most recent head d55af0e. Consider uploading reports for the commit d55af0e to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #464      +/-   ##
==========================================
- Coverage   71.81%   71.80%   -0.02%     
==========================================
  Files          84       84              
  Lines        5745     5746       +1     
  Branches     1235     1235              
==========================================
  Hits         4126     4126              
- Misses       1339     1340       +1     
  Partials      280      280              

Files Changed	Coverage Δ
lona/app.py	58.75% <ø> (ø)
lona/command_line/handle_command_line.py	0.00% <0.00%> (ø)

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

[fscherf]

@AvdN: Works like a charm! I added a small fix to your patch because LONA_DEFAULT_HOST did only work for apps, not for projects. Let me know if you are ok with this change, then we can merge.

[AvdN]

I am not sure if I am supposed to be able to see the changes on github, at least I cannot find them. But go ahead, I'll see them at the latest when they are merged.

[fscherf]

@AvdN: Thanks for contributing! :)