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

More gracefully override JPS ExtensionApp.config_file_paths #612

Merged
merged 2 commits into from
Jul 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 17 additions & 10 deletions cylc/uiserver/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
from pathlib import Path, PurePath
import sys
from textwrap import dedent
from typing import List
from typing import List, Optional

from pkg_resources import parse_version
from tornado import ioloop
Expand Down Expand Up @@ -145,15 +145,6 @@ class CylcUIServer(ExtensionApp):
cylc gui --no-browser # Start the server but don't open the browser

''') # type: ignore[assignment]
config_file_paths = get_conf_dir_hierarchy(
[
SITE_CONF_ROOT, # site configuration
USER_CONF_ROOT, # user configuration
], filename=False
)
# Next include currently needed for directory making
config_file_paths.insert(0, str(Path(uis_pkg).parent)) # packaged config
config_file_paths.reverse()
# TODO: Add a link to the access group table mappings in cylc documentation
# https://github.com/cylc/cylc-uiserver/issues/466
AUTH_DESCRIPTION = '''
Expand Down Expand Up @@ -405,6 +396,7 @@ def _get_ui_path(self):

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self._config_file_paths: Optional[List[str]] = None
self.executor = ProcessPoolExecutor(max_workers=self.max_workers)
self.workflows_mgr = WorkflowsManager(self, log=self.log)
self.data_store_mgr = DataStoreMgr(
Expand All @@ -421,6 +413,21 @@ def __init__(self, *args, **kwargs):
workflows_mgr=self.workflows_mgr,
)

@property
def config_file_paths(self) -> List[str]:
if self._config_file_paths is None:
ret = get_conf_dir_hierarchy(
[
SITE_CONF_ROOT, # site configuration
USER_CONF_ROOT, # user configuration
], filename=False
)
# Next include currently needed for directory making
ret.insert(0, str(Path(uis_pkg).parent)) # packaged config
ret.reverse()
self._config_file_paths = ret
return self._config_file_paths
Comment on lines +416 to +429
Copy link
Member

Choose a reason for hiding this comment

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

This change does not appear to be necessary, the only bit that's needed is the autouse?

Copy link
Member Author

@MetRonnie MetRonnie Jul 9, 2024

Choose a reason for hiding this comment

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

Couldn't get the monkeypatching to work without this change. Not sure why, but pyright is not happy with overriding a property with a class attribute anyway (see microsoft/pyright#3646 (comment))

Copy link
Member

Choose a reason for hiding this comment

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

Monkeypatching seems to work fine for me (with your config)?

I found the config_file_paths definition in Jupyter_Core and it is indeed a property so this makes sense 👍.


def initialize_settings(self):
"""Update extension settings.

Expand Down
16 changes: 6 additions & 10 deletions cylc/uiserver/tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,14 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
"""Test code and fixtures."""

from getpass import getuser
import inspect
import logging
from pathlib import Path
from shutil import rmtree
from socket import gethostname
from tempfile import TemporaryDirectory
from uuid import uuid4

import pytest
from tornado.web import HTTPError
from traitlets.config import Config
import zmq

Expand All @@ -47,6 +44,7 @@
from cylc.flow.parsec.config import ParsecConfig
from cylc.flow.parsec.validate import cylc_config_validate


class AsyncClientFixture(WorkflowRuntimeClient):
pattern = zmq.REQ
host = ''
Expand Down Expand Up @@ -223,7 +221,7 @@ def mock_authentication_yossarian(monkeypatch):
def jp_server_config(jp_template_dir):
"""Config to turn the CylcUIServer extension on.

Auto-loading, add as an argument in the test function to activate.
Overrides jupyter server fixture of the same name.
"""
config = {
"ServerApp": {
Expand All @@ -235,12 +233,9 @@ def jp_server_config(jp_template_dir):
return config


@pytest.fixture
def patch_conf_files(monkeypatch):
"""Auto-patches the CylcUIServer to prevent it loading config files.

Auto-loading, add as an argument in the test function to activate.
"""
@pytest.fixture(autouse=True)
def patch_conf_files(monkeypatch: pytest.MonkeyPatch):
"""Auto-patches the CylcUIServer to prevent it loading config files."""
monkeypatch.setattr(
'cylc.uiserver.app.CylcUIServer.config_file_paths', []
)
Expand Down Expand Up @@ -364,6 +359,7 @@ def workflow_run_dir(request):
if not request.session.testsfailed:
rmtree(run_dir)


@pytest.fixture
def mock_glbl_cfg(tmp_path: Path, monkeypatch: pytest.MonkeyPatch):
"""A Pytest fixture for fiddling global config values.
Expand Down
4 changes: 1 addition & 3 deletions cylc/uiserver/tests/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

@pytest.mark.integration
@pytest.mark.usefixtures("mock_authentication_yossarian")
async def test_cylc_handler(patch_conf_files, jp_fetch):
async def test_cylc_handler(jp_fetch):
"""The Cylc endpoints have been added and work."""
resp = await jp_fetch(
'cylc', 'userprofile', method='GET'
Expand Down Expand Up @@ -60,7 +60,6 @@ async def test_cylc_handler(patch_conf_files, jp_fetch):
]
)
async def test_authorised_and_authenticated(
patch_conf_files,
jp_fetch,
endpoint,
code,
Expand Down Expand Up @@ -95,7 +94,6 @@ async def test_authorised_and_authenticated(
]
)
async def test_unauthorised(
patch_conf_files,
jp_fetch,
endpoint,
code,
Expand Down
Loading