-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/address on local #256
Changes from all commits
0b44822
d2712ab
002b9e9
d7284d0
9a8c5df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -157,4 +157,10 @@ token.txt | |
!/.github/* | ||
|
||
# data_store | ||
/data_store | ||
/data_store | ||
|
||
# Mac OS | ||
.DS_Store | ||
|
||
# Coverage | ||
cov.xml |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -324,14 +324,16 @@ def install(debug: bool, force: bool, directory: str, version: str): | |
@registry.command() | ||
@click.option("--debug/--no-debug", help="Run in debug mode", default=False) | ||
@click.option("--port", help="port on which to run registry", default=8000) | ||
def start(debug: bool, port: int) -> None: | ||
@click.option("--address", help="Address on which to run registry", default="127.0.0.1") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Win 10, Py 10 Works with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an issue with allowed hosts: base_settings.py The address will need adding to the allowed hosts or allowing all hosts. This can be added to local-settings.py Or we could import local settings into a new settings file and put it there. Open to suggestions on where and how we add this in? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi sorry if I jump in. I would not allow any hosts. I would edit the Django settings to be like ALLOWED_HOSTS = [] And then pass allowed host as env variable. This would allow to have less maintenance overhead in the Django app for future changes. You could also create a sys-env variable (prod/testing/staging) and have different Django settings. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My intuition here is that this feature is going to be used infrequently or by expert users for either running in containers or deploying a "remote" registry. @bruvio's suggestion looks good (with my limited Django). Thanks! I agree that allowing all hosts by default is unlikely to be good practise. I think if you go with the suggestion, it should be documented somewhere. I guess the other way might be to just document what manual changes are needed to these files to allow the host specified by the CLI. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thanks @bruvio Agree this is a good solution, I would probably set allowed hosts in the settings to ['127.0.0.1', 'localhost'] prior to extending it with the environment variable so if the environmental variable is not set, it will still run on both. I'll put in a PR on the registry and we can add the address to the environment variable in the start registry function |
||
def start(debug: bool, port: int, address: str) -> None: | ||
"""Start the local registry server""" | ||
try: | ||
fdp_session.FAIR( | ||
os.getcwd(), | ||
server_mode=fdp_svr.SwitchMode.USER_START, | ||
debug=debug, | ||
server_port=port, | ||
server_address=address | ||
) | ||
except fdp_exc.FAIRCLIException as e: | ||
if debug: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,12 +38,15 @@ | |
import enum | ||
import os | ||
import pathlib | ||
import logging | ||
|
||
import git | ||
import yaml | ||
|
||
import fair.exceptions as fdp_exc | ||
|
||
_logger = logging.getLogger("FAIRDataPipeline.Common") | ||
|
||
USER_FAIR_DIR = os.path.join(pathlib.Path.home(), ".fair") | ||
FAIR_CLI_CONFIG = "cli-config.yaml" | ||
USER_CONFIG_FILE = "config.yaml" | ||
|
@@ -145,6 +148,23 @@ def registry_session_port_file(registry_dir: str = None) -> str: | |
registry_dir = registry_home() | ||
return os.path.join(registry_dir, "session_port.log") | ||
|
||
def registry_session_address_file(registry_dir: str = None) -> str: | ||
"""Retrieve the location of the registry session port file | ||
|
||
Parameters | ||
---------- | ||
registry_dir : str, optional | ||
registry directory, by default None | ||
|
||
Returns | ||
------- | ||
str | ||
path to registry port session file | ||
""" | ||
if not registry_dir: | ||
registry_dir = registry_home() | ||
return os.path.join(registry_dir, "session_address.log") | ||
|
||
|
||
def registry_session_port(registry_dir: str = None) -> int: | ||
"""Retrieve the registry session port | ||
|
@@ -164,6 +184,33 @@ def registry_session_port(registry_dir: str = None) -> int: | |
""" | ||
return int(open(registry_session_port_file(registry_dir)).read().strip()) | ||
|
||
def registry_session_address(registry_dir: str = None) -> str: | ||
"""Retrieve the registry session address | ||
|
||
Unlike 'get_local_address' within the configuration module, this retrieves the | ||
port number from the file generated by the registry itself | ||
|
||
Parameters | ||
---------- | ||
registry_dir : str, optional | ||
registry directory, by default None | ||
|
||
Returns | ||
------- | ||
str | ||
current/most recent address used to launch the registry | ||
""" | ||
if not os.path.exists(registry_session_address_file(registry_dir)): | ||
_logger.warning("Session Address file not found, please make sure your registry is up-to-date") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this could happen with an older registry version running. |
||
_logger.info("Using 127.0.0.1") | ||
return "127.0.0.1" | ||
|
||
_address = open(registry_session_address_file(registry_dir)).read().strip() | ||
if _address != "0.0.0.0": | ||
return _address | ||
else: | ||
return "127.0.0.1" | ||
|
||
|
||
def staging_cache(user_loc: str) -> str: | ||
"""Location of staging cache for the given repository""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this option is working, but I can only get the registry to start on
127.0.0.1
.