Feature/address on local #256

RyanJField · 2022-07-21T13:11:00Z

Implemented address parameter to allow the local registry to be bound to an alternative address (interface).

Fixed check_grid as the grid api has been moved over to ROR

bobturneruk · 2022-07-28T12:43:08Z

Having some difficulty getting going on Windows 10 - is this expected to work for all current Python versions? Could be my environment / use of conda / having installed many previous versions.

bobturneruk · 2022-07-28T13:33:31Z

@RyanJField currently getting (win 10, py 3.9):

(fdp) PS C:\Users\bobturner\Documents\FAIR-CLI> fair registry install
Traceback (most recent call last):
  File "C:\Users\bobturner\anaconda3\envs\fdp\lib\runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "C:\Users\bobturner\anaconda3\envs\fdp\lib\runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "C:\Users\bobturner\anaconda3\envs\fdp\Scripts\fair.exe\__main__.py", line 7, in <module>
  File "C:\Users\bobturner\anaconda3\envs\fdp\lib\site-packages\click\core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "C:\Users\bobturner\anaconda3\envs\fdp\lib\site-packages\click\core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "C:\Users\bobturner\anaconda3\envs\fdp\lib\site-packages\click\core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "C:\Users\bobturner\anaconda3\envs\fdp\lib\site-packages\click\core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "C:\Users\bobturner\anaconda3\envs\fdp\lib\site-packages\click\core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "C:\Users\bobturner\anaconda3\envs\fdp\lib\site-packages\click\core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "C:\Users\bobturner\Documents\FAIR-CLI\fair\cli.py", line 312, in install
    _version = fdp_svr.install_registry(
  File "C:\Users\bobturner\Documents\FAIR-CLI\fair\registry\server.py", line 363, in install_registry
    _venv.create(venv_dir)
  File "C:\Users\bobturner\anaconda3\envs\fdp\lib\venv\__init__.py", line 75, in create
    self._setup_pip(context)
  File "C:\Users\bobturner\anaconda3\envs\fdp\lib\venv\__init__.py", line 313, in _setup_pip
    subprocess.check_output(cmd, stderr=subprocess.STDOUT)
  File "C:\Users\bobturner\anaconda3\envs\fdp\lib\subprocess.py", line 424, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "C:\Users\bobturner\anaconda3\envs\fdp\lib\subprocess.py", line 528, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['C:\\Users\\bobturner\\.fair\\registry\\venv\\Scripts\\python.exe', '-Im', 'ensurepip', '--upgrade', '--default-pip']' returned non-zero exit status 9009.

using develop branch of FAIR-CLI to install.

bobturneruk · 2022-07-28T13:39:00Z

Registry install (fair registry install) seems to work OK if FARI-CLI is installed from PyPI (pip install fair-cli), not GitHub.

bobturneruk · 2022-07-28T13:42:52Z

The differences between develop and the current PyPI release are substantial v0.2.3...develop

Should I install the registry via FAIR-CLI from PyPI and then reinstall FAIR-CLI from this branch to review this PR, or is it best to address the install issue with develop first? cc @RyanJField , @richardreeve Or maybe it's just me 😄!

RyanJField · 2022-07-29T14:50:19Z

The version on PyPi for windows should use the main branch of the registry, however the version on GitHub uses the latest tagged release (unless otherwise specified) which is currently broken for windows 10 python 3.10, this PR fixed it FAIRDataPipeline/data-registry@971cde5 but I need to push a new tagged release and I'm waiting for this PR to be approved first: FAIRDataPipeline/data-registry#191 so that there is only one release (and not two).

Hope this makes sense.

Once the push functionality is working and the CLI is better tested we should update the PyPi release. The GitHub version of the registry requires extra steps (specifying the remote registry or using the local flag) than the PyPi version so we were waiting for the paper to be accepted to update it (incase any of the reviewers wanted to run the code).

bobturneruk · 2022-08-01T09:37:13Z

Cheers - I'll look at FAIRDataPipeline/data-registry#191 first, then.

bobturneruk · 2022-08-11T12:23:34Z

Should I wait for #257 to be merged before looking at this again, or install off hotfix/python-exec-fix then switch to feature/address_on_local?

bobturneruk · 2022-08-11T12:24:19Z

Or is this (#256) now the blocker?

RyanJField · 2022-08-11T12:43:00Z

Or is this (#256) now the blocker?

I have merged #257 this (#256) needs reviewing and merging to fix the tests now.

bobturneruk

These changes look good, in themselves. I don't know Django or a huge amount about big design decisions made earlier on in this repo's history so can't properly review in context.

bobturneruk · 2022-08-05T14:00:08Z

fair/cli.py

@@ -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")


I think this option is working, but I can only get the registry to start on 127.0.0.1.

bobturneruk · 2022-08-15T09:32:57Z

fair/cli.py

@@ -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")


Win 10, Py 10

Works with --address 127.0.0.1 not with 192.168.1.101 my laptop address at the time - gives Bad Request (400) so serving something! The bit, the click CLI is clearly OK.

This is an issue with allowed hosts: base_settings.py

The address will need adding to the allowed hosts or allowing all hosts.
ALLOWED_HOSTS = ['*']
ALLOWED_HOSTS = ['127.0.0.1', 'localhost', '192.168.0.101']

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?

Hi sorry if I jump in.

I would not allow any hosts.

I would edit the Django settings to be like

ALLOWED_HOSTS = []
ALLOWED_HOSTS.extend(
filter(None, os.environ.get("ALLOWED_HOSTS", "").split(","))
)

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.

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.

Hi sorry if I jump in.

I would not allow any hosts.

I would edit the Django settings to be like

ALLOWED_HOSTS = [] ALLOWED_HOSTS.extend( filter(None, os.environ.get("ALLOWED_HOSTS", "").split(",")) )

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.

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

bobturneruk · 2022-08-15T09:36:08Z

fair/common.py

+        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")


I guess this could happen with an older registry version running.

fair/identifiers.py

bobturneruk · 2022-08-15T09:49:07Z

tests/test_server.py

+    with tempfile.TemporaryDirectory() as tempd:
+        reg_dir = os.path.join(tempd, "registry")
+        mocker.patch("fair.common.DEFAULT_REGISTRY_LOCATION", reg_dir)
+        fdp_serv.install_registry(install_dir=reg_dir)


Maybe there's a fixture that could be used in situations like this to avoid multiple installs.

There is a registry fixture but it cannot be used for these commands due to an issue with the virtual environment it uses. To rewrite it would take time.

Co-authored-by: bobturneruk <34244196+bobturneruk@users.noreply.github.com>

codecov · 2022-08-15T11:34:11Z

Codecov Report

Merging #256 (9a8c5df) into develop (cbaadf1) will increase coverage by 0.17%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #256      +/-   ##
===========================================
+ Coverage    69.38%   69.55%   +0.17%     
===========================================
  Files           27       27              
  Lines         3642     3653      +11     
===========================================
+ Hits          2527     2541      +14     
+ Misses        1115     1112       -3

Impacted Files	Coverage Δ
fair/cli.py	`51.49% <100.00%> (+0.13%)`	⬆️
fair/common.py	`93.87% <100.00%> (+1.10%)`	⬆️
fair/configuration/__init__.py	`69.78% <100.00%> (+0.02%)`	⬆️
fair/identifiers.py	`100.00% <100.00%> (+3.07%)`	⬆️
fair/registry/server.py	`84.61% <100.00%> (+0.08%)`	⬆️
fair/session.py	`54.26% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

sonarcloud · 2022-08-15T16:32:58Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

No Coverage information
0.0% Duplication

RyanJField added 3 commits July 19, 2022 11:35

Add Address option to local registry

0b44822

Move Grid API to ROR

d2712ab

Refactor Identifiers to reduce complexity

002b9e9

RyanJField assigned richardreeve, bruvio and bobturneruk Jul 21, 2022

RyanJField requested a review from bobturneruk July 27, 2022 15:18

RyanJField unassigned richardreeve, bruvio and bobturneruk Jul 27, 2022

RyanJField mentioned this pull request Aug 10, 2022

Fix Registry install on windows #257

Merged

bobturneruk approved these changes Aug 15, 2022

View reviewed changes

Update fair/identifiers.py

d7284d0

Co-authored-by: bobturneruk <34244196+bobturneruk@users.noreply.github.com>

RyanJField mentioned this pull request Aug 15, 2022

Add environment variable to allowed hosts FAIRDataPipeline/data-registry#193

Merged

Add FAIR_ALLOWED_HOSTS Variable

9a8c5df

RyanJField merged commit 016dd57 into develop Aug 16, 2022

RyanJField deleted the feature/address_on_local branch August 16, 2022 09:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/address on local #256

Feature/address on local #256

RyanJField commented Jul 21, 2022

bobturneruk commented Jul 28, 2022

bobturneruk commented Jul 28, 2022 •

edited

Loading

bobturneruk commented Jul 28, 2022

bobturneruk commented Jul 28, 2022

RyanJField commented Jul 29, 2022

bobturneruk commented Aug 1, 2022

bobturneruk commented Aug 11, 2022

bobturneruk commented Aug 11, 2022

RyanJField commented Aug 11, 2022

bobturneruk left a comment

bobturneruk Aug 5, 2022

bobturneruk Aug 15, 2022

RyanJField Aug 15, 2022

bruvio Aug 15, 2022

bobturneruk Aug 15, 2022

RyanJField Aug 15, 2022

bobturneruk Aug 15, 2022

bobturneruk Aug 15, 2022

RyanJField Aug 15, 2022

codecov bot commented Aug 15, 2022 •

edited

Loading

sonarcloud bot commented Aug 15, 2022

Feature/address on local #256

Feature/address on local #256

Conversation

RyanJField commented Jul 21, 2022

bobturneruk commented Jul 28, 2022

bobturneruk commented Jul 28, 2022 • edited Loading

bobturneruk commented Jul 28, 2022

bobturneruk commented Jul 28, 2022

RyanJField commented Jul 29, 2022

bobturneruk commented Aug 1, 2022

bobturneruk commented Aug 11, 2022

bobturneruk commented Aug 11, 2022

RyanJField commented Aug 11, 2022

bobturneruk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 15, 2022 • edited Loading

Codecov Report

sonarcloud bot commented Aug 15, 2022

bobturneruk commented Jul 28, 2022 •

edited

Loading

codecov bot commented Aug 15, 2022 •

edited

Loading