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 base URL to configuration file #135

Merged
merged 6 commits into from Jan 20, 2020
Merged

Add base URL to configuration file #135

merged 6 commits into from Jan 20, 2020

Conversation

CasperWA
Copy link
Member

Makes it possible to set the base URL from the config file. Should otherwise use the base URL gotten from parsing the requested URL (which may be "wrong" if run as a proxy).

@CasperWA CasperWA added the enhancement New feature or request label Jan 13, 2020
@codecov
Copy link

codecov bot commented Jan 13, 2020

Codecov Report

Merging #135 into master will increase coverage by 0.18%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
+ Coverage   85.79%   85.98%   +0.18%     
==========================================
  Files          39       39              
  Lines        1830     1826       -4     
==========================================
  Hits         1570     1570              
+ Misses        260      256       -4
Flag Coverage Δ
#unittests 85.98% <100%> (+0.18%) ⬆️
Impacted Files Coverage Δ
optimade/server/routers/utils.py 83.73% <100%> (+0.26%) ⬆️
optimade/server/config.py 93.13% <100%> (+3.41%) ⬆️
optimade/server/routers/info.py 96.29% <100%> (-0.14%) ⬇️

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 f719b14...8d9a6c3. Read the comment docs.

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks @CasperWA !

Besides the comments I have two more questions:

  • I tried out your PR since I wanted to check what value the base URL takes if it is parsed from the request. This showed me that I actually don't know where this base URL is actually returned by the API. Can you give an example?
  • I think the config.ini file should be at the top level of the repo. Since it is a configuration file, it should not be considered part of the code

optimade/server/config.ini Outdated Show resolved Hide resolved
optimade/server/config.py Outdated Show resolved Hide resolved
base_url = (
CONFIG.base_url
if CONFIG.base_url
else f"{parse_result.scheme}://{parse_result.netloc}"
Copy link
Member

Choose a reason for hiding this comment

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

Oh, so you get the base URL from the request?

I wonder - would that solve the problem of having to specify the base URL?

Copy link
Member Author

Choose a reason for hiding this comment

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

No.
The else value is what used to be up to now, and is giving false information if run in a docker (it will provide you with something like http://localhost:3585 or whatever the port is).

optimade/server/routers/utils.py Outdated Show resolved Hide resolved
@CasperWA
Copy link
Member Author

I tried out your PR since I wanted to check what value the base URL takes if it is parsed from the request. This showed me that I actually don't know where this base URL is actually returned by the API. Can you give an example?

Sure.
The easiest is to go to the structures endpoint and set a "low" page_limit, e.g., 5.
Then you will see the result of the base URL under the links.next.

I think the config.ini file should be at the top level of the repo. Since it is a configuration file, it should not be considered part of the code

Yeah - it's not a bad idea to combine this PR with fixing #134.
We can use the same implementation/way to find the config file as is used to find the server.cfg file now, i.e., look in the cwd.

@CasperWA CasperWA changed the title Add base URL to configuration file Add base URL to configuration file - rework config.py to not use server.cfg Jan 13, 2020
@CasperWA CasperWA changed the title Add base URL to configuration file - rework config.py to not use server.cfg [WIP] Add base URL to configuration file - rework config.py to not use server.cfg Jan 13, 2020
@ltalirz
Copy link
Member

ltalirz commented Jan 13, 2020

The easiest is to go to the structures endpoint and set a "low" page_limit, e.g., 5.
Then you will see the result of the base URL under the links.next.

thanks, I tried setting it to 50 but I still see next: null in the json output

@CasperWA CasperWA changed the base branch from master to develop January 14, 2020 19:17
@CasperWA CasperWA changed the base branch from develop to master January 15, 2020 11:29
CasperWA and others added 2 commits January 20, 2020 12:35
The default index_base_url for a provider is now `None` (or `null`).
@CasperWA CasperWA requested a review from ltalirz January 20, 2020 11:46
@CasperWA CasperWA changed the title [WIP] Add base URL to configuration file - rework config.py to not use server.cfg Add base URL to configuration file Jan 20, 2020
@CasperWA CasperWA mentioned this pull request Jan 20, 2020
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks @CasperWA !

@CasperWA CasperWA merged commit c6d371c into master Jan 20, 2020
@CasperWA CasperWA deleted the add_base_url_to_config branch January 20, 2020 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants