From a5a13ec54f41cae4f75ba977663a1c65825b4d5f Mon Sep 17 00:00:00 2001 From: Erik Sundell Date: Fri, 24 Mar 2023 08:56:32 +0100 Subject: [PATCH 01/48] Empty dummy commit to allow a draft PR to be created From bb910cdde29a99d87984c963bacfda9ec775ff9f Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Thu, 6 Apr 2023 12:57:33 +0300 Subject: [PATCH 02/48] Add the traitlet option and check query param with same name --- binderhub/app.py | 6 +++ binderhub/builder.py | 87 ++++++++++++++++++++++++++------------------ 2 files changed, 57 insertions(+), 36 deletions(-) diff --git a/binderhub/app.py b/binderhub/app.py index da1e377e5..922fabf85 100644 --- a/binderhub/app.py +++ b/binderhub/app.py @@ -786,6 +786,11 @@ def _template_path_default(self): help="Origin to use when emitting events. Defaults to hostname of request when empty", ) + no_launch = Bool( + False, + help="When enabled, the hub will no longer launch the image after the build" + ) + _build_config_deprecated_map = { "appendix": ("BuildExecutor", "appendix"), "push_secret": ("BuildExecutor", "push_secret"), @@ -943,6 +948,7 @@ def initialize(self, *args, **kwargs): "auth_enabled": self.auth_enabled, "event_log": self.event_log, "normalized_origin": self.normalized_origin, + "no_launch": self.no_launch, } ) if self.auth_enabled: diff --git a/binderhub/builder.py b/binderhub/builder.py index 4fd34579f..d4fad12a4 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -408,33 +408,47 @@ async def get(self, provider_prefix, _unescaped_spec): else: image_found = True - if image_found: + no_launch = self.settings.get("no_launch", False) + no_launch_req_query_attr = self.get_query_argument(name="no-launch", default="") + # The request query attribute takes precedence over the traitlet config + if no_launch_req_query_attr: + no_launch = True + if no_launch: await self.emit( { - "phase": "built", + "phase": "info", "imageName": image_name, - "message": "Found built image, launching...\n", + "message": f"Found `no_launch` option. Image will not be launched after build.\n", } ) - with LAUNCHES_INPROGRESS.track_inprogress(): - try: - await self.launch(provider) - except LaunchQuotaExceeded: - return - self.event_log.emit( - "binderhub.jupyter.org/launch", - 5, + if image_found: + await self.emit( { - "provider": provider.name, - "spec": spec, - "ref": ref, - "status": "success", - "build_token": self._have_build_token, - "origin": self.settings["normalized_origin"] - if self.settings["normalized_origin"] - else self.request.host, - }, + "phase": "built", + "imageName": image_name, + "message": "Found built image\n", + } ) + if not no_launch: + with LAUNCHES_INPROGRESS.track_inprogress(): + try: + await self.launch(provider) + except LaunchQuotaExceeded: + return + self.event_log.emit( + "binderhub.jupyter.org/launch", + 5, + { + "provider": provider.name, + "spec": spec, + "ref": ref, + "status": "success", + "build_token": self._have_build_token, + "origin": self.settings["normalized_origin"] + if self.settings["normalized_origin"] + else self.request.host, + }, + ) return # Don't allow builds when quota is exceeded @@ -515,7 +529,7 @@ def _check_result(future): elif progress.payload == ProgressEvent.BuildStatus.BUILT: event = { "phase": phase, - "message": "Built image, launching...\n", + "message": "Done! Image built.\n", "imageName": image_name, } done = True @@ -559,21 +573,22 @@ def _check_result(future): ) BUILD_COUNT.labels(status="success", **self.repo_metric_labels).inc() with LAUNCHES_INPROGRESS.track_inprogress(): - await self.launch(provider) - self.event_log.emit( - "binderhub.jupyter.org/launch", - 5, - { - "provider": provider.name, - "spec": spec, - "ref": ref, - "status": "success", - "build_token": self._have_build_token, - "origin": self.settings["normalized_origin"] - if self.settings["normalized_origin"] - else self.request.host, - }, - ) + if not no_launch: + await self.launch(provider) + self.event_log.emit( + "binderhub.jupyter.org/launch", + 5, + { + "provider": provider.name, + "spec": spec, + "ref": ref, + "status": "success", + "build_token": self._have_build_token, + "origin": self.settings["normalized_origin"] + if self.settings["normalized_origin"] + else self.request.host, + }, + ) # Don't close the eventstream immediately. # (javascript) eventstream clients reconnect automatically on dropped connections, From 2ed3e8b8665f1400bd74c08b7c6a0f1a232c3674 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Thu, 6 Apr 2023 12:57:57 +0300 Subject: [PATCH 03/48] Update api exaple to support option --- binderhub/app.py | 2 +- examples/binder-api.py | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/binderhub/app.py b/binderhub/app.py index 922fabf85..db413ecfb 100644 --- a/binderhub/app.py +++ b/binderhub/app.py @@ -788,7 +788,7 @@ def _template_path_default(self): no_launch = Bool( False, - help="When enabled, the hub will no longer launch the image after the build" + help="When enabled, the hub will no longer launch the image after the build", ) _build_config_deprecated_map = { diff --git a/examples/binder-api.py b/examples/binder-api.py index a066c81ed..a2cd1cb44 100644 --- a/examples/binder-api.py +++ b/examples/binder-api.py @@ -15,14 +15,18 @@ import requests -def build_binder(repo, ref, *, binder_url="https://mybinder.org"): +def build_binder(repo, ref, *, binder_url="https://mybinder.org", no_launch): """Launch a binder Yields Binder's event-stream events (dicts) """ print(f"Building binder for {repo}@{ref}") url = binder_url + f"/build/gh/{repo}/{ref}" - r = requests.get(url, stream=True) + params = {} + if no_launch: + params = {"no-launch": "True"} + + r = requests.get(url, stream=True, params=params) r.raise_for_status() for line in r.iter_lines(): line = line.decode("utf8", "replace") @@ -34,12 +38,17 @@ def build_binder(repo, ref, *, binder_url="https://mybinder.org"): parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("repo", type=str, help="The GitHub repo to build") parser.add_argument("--ref", default="HEAD", help="The ref of the repo to build") + parser.add_argument( + "--no-launch", + action="store_true", + help="When passed, the image will not be launched after build", + ) file_or_url = parser.add_mutually_exclusive_group() file_or_url.add_argument("--filepath", type=str, help="The file to open, if any.") file_or_url.add_argument("--urlpath", type=str, help="The url to open, if any.") parser.add_argument( "--binder", - default="https://mybinder.org", + default="http://localhost:8585", help=""" The URL of the binder instance to use. Use `http://localhost:8585` if you are doing local testing. @@ -47,7 +56,9 @@ def build_binder(repo, ref, *, binder_url="https://mybinder.org"): ) opts = parser.parse_args() - for evt in build_binder(opts.repo, ref=opts.ref, binder_url=opts.binder): + for evt in build_binder( + opts.repo, ref=opts.ref, binder_url=opts.binder, no_launch=opts.no_launch + ): if "message" in evt: print( "[{phase}] {message}".format( From 4a116380a421f6aadaa9019d94a89ce8c469cc1d Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Thu, 6 Apr 2023 13:02:31 +0300 Subject: [PATCH 04/48] No need for fstrings --- binderhub/builder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index d4fad12a4..9dc872032 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -418,7 +418,7 @@ async def get(self, provider_prefix, _unescaped_spec): { "phase": "info", "imageName": image_name, - "message": f"Found `no_launch` option. Image will not be launched after build.\n", + "message": "Found `no_launch` option. Image will not be launched after build.\n", } ) if image_found: From 008cc3490d6c58796767192ac491b631fb2b9978 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 7 Apr 2023 10:50:37 +0300 Subject: [PATCH 05/48] Try out a test --- binderhub/builder.py | 2 +- binderhub/tests/test_build.py | 44 +++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index 9dc872032..2f71b87e2 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -418,7 +418,7 @@ async def get(self, provider_prefix, _unescaped_spec): { "phase": "info", "imageName": image_name, - "message": "Found `no_launch` option. Image will not be launched after build.\n", + "message": "Found no launch option. Image will not be launched after build.\n", } ) if image_found: diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index 7f7a795ea..72264f3c8 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -120,6 +120,50 @@ async def test_build_fail(app, needs_build, needs_launch, always_build, pytestco assert failed_events > 0, "Should have seen phase 'failed'" +@pytest.mark.asyncio(timeout=120) +@pytest.mark.remote +async def test_build_no_launch(app): + """ + Test build endpoint with no launch option active, passed as a request query param. + """ + slug = "gh/binderhub-ci-repos/cached-minimal-dockerfile/HEAD" + build_url = f"{app.url}/build/{slug}" + params = {} + params = {"no-launch": "True"} + r = await async_requests.get(build_url, stream=True, params=params) + r.raise_for_status() + + events = [] + launch_events = 0 + async for line in async_requests.iter_lines(r): + line = line.decode("utf8", "replace") + if line.startswith("data:"): + event = json.loads(line.split(":", 1)[1]) + events.append(event) + assert "message" in event + sys.stdout.write(f"{event.get('phase', '')}: {event['message']}") + if event.get("phase") == "info": + assert ( + "Found no launch option. Image will not be launched after build" + in event["message"] + ) + # This is the signal that the image was built. + # Check the message for clue that the image won't be launched and + # break out of the loop now because BinderHub keeps the connection open + # for many seconds after to avoid "reconnects" from slow clients + if event.get("phase") == "built": + assert "Ready! Image won't be launched" in event["message"] + r.close() + break + if event.get("phase") == "launching": + launch_events += 1 + + assert launch_events == 0 + final = events[-1] + assert "phase" in final + assert final["phase"] == "built" + + def _list_image_builder_pods_mock(): """Mock list of DIND pods""" mock_response = mock.MagicMock() From 3505fcb02725eebe8bd8ce607c93590ce6da7a35 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 7 Apr 2023 10:56:37 +0300 Subject: [PATCH 06/48] Update messages emmited --- binderhub/builder.py | 16 ++++++++++++---- binderhub/tests/test_build.py | 2 +- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index 2f71b87e2..f7c735697 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -429,12 +429,12 @@ async def get(self, provider_prefix, _unescaped_spec): "message": "Found built image\n", } ) - if not no_launch: - with LAUNCHES_INPROGRESS.track_inprogress(): + with LAUNCHES_INPROGRESS.track_inprogress(): + if not no_launch: try: await self.launch(provider) except LaunchQuotaExceeded: - return + return self.event_log.emit( "binderhub.jupyter.org/launch", 5, @@ -573,7 +573,15 @@ def _check_result(future): ) BUILD_COUNT.labels(status="success", **self.repo_metric_labels).inc() with LAUNCHES_INPROGRESS.track_inprogress(): - if not no_launch: + if no_launch: + await self.emit( + { + "phase": "built", + "imageName": image_name, + "message": "Image won't be launched\n", + } + ) + else: await self.launch(provider) self.event_log.emit( "binderhub.jupyter.org/launch", diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index 72264f3c8..28699ccd0 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -152,7 +152,7 @@ async def test_build_no_launch(app): # break out of the loop now because BinderHub keeps the connection open # for many seconds after to avoid "reconnects" from slow clients if event.get("phase") == "built": - assert "Ready! Image won't be launched" in event["message"] + assert "Image won't be launched" in event["message"] r.close() break if event.get("phase") == "launching": From 5ec3c5af7d3eb1434423f230557bb9bba6428b69 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 7 Apr 2023 11:15:11 +0300 Subject: [PATCH 07/48] Use the ready phase to signal end of ops --- binderhub/builder.py | 14 +++++++++++--- binderhub/tests/test_build.py | 4 ++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index f7c735697..590e882ff 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -430,11 +430,19 @@ async def get(self, provider_prefix, _unescaped_spec): } ) with LAUNCHES_INPROGRESS.track_inprogress(): - if not no_launch: + if no_launch: + await self.emit( + { + "phase": "built", + "imageName": image_name, + "message": "Image won't be launched\n", + } + ) + else: try: await self.launch(provider) except LaunchQuotaExceeded: - return + return self.event_log.emit( "binderhub.jupyter.org/launch", 5, @@ -576,7 +584,7 @@ def _check_result(future): if no_launch: await self.emit( { - "phase": "built", + "phase": "ready", "imageName": image_name, "message": "Image won't be launched\n", } diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index 28699ccd0..f9b6ae6f3 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -151,7 +151,7 @@ async def test_build_no_launch(app): # Check the message for clue that the image won't be launched and # break out of the loop now because BinderHub keeps the connection open # for many seconds after to avoid "reconnects" from slow clients - if event.get("phase") == "built": + if event.get("phase") == "ready": assert "Image won't be launched" in event["message"] r.close() break @@ -161,7 +161,7 @@ async def test_build_no_launch(app): assert launch_events == 0 final = events[-1] assert "phase" in final - assert final["phase"] == "built" + assert final["phase"] == "ready" def _list_image_builder_pods_mock(): From bc8f68c9e485e8076c599db96afd57fab37eaef2 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 7 Apr 2023 12:38:26 +0300 Subject: [PATCH 08/48] Make the no_launch option configurable --- binderhub/app.py | 1 + 1 file changed, 1 insertion(+) diff --git a/binderhub/app.py b/binderhub/app.py index db413ecfb..f9676ca1c 100644 --- a/binderhub/app.py +++ b/binderhub/app.py @@ -789,6 +789,7 @@ def _template_path_default(self): no_launch = Bool( False, help="When enabled, the hub will no longer launch the image after the build", + config=True, ) _build_config_deprecated_map = { From 5987b4e44cfd4e8d6b775bf39a678a92d2862a13 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 7 Apr 2023 12:41:41 +0300 Subject: [PATCH 09/48] Add no_launch to local testing config --- testing/local-binder-mocked-hub/binderhub_config.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/testing/local-binder-mocked-hub/binderhub_config.py b/testing/local-binder-mocked-hub/binderhub_config.py index a9a5c0584..d5d341723 100644 --- a/testing/local-binder-mocked-hub/binderhub_config.py +++ b/testing/local-binder-mocked-hub/binderhub_config.py @@ -16,6 +16,8 @@ c.BinderHub.builder_required = False c.BinderHub.repo_providers = {"gh": FakeProvider} c.BinderHub.build_class = FakeBuild +# Uncomment the following line to enable BinderHub to not launch the image after build +# c.BinderHub.no_launch = True c.BinderHub.about_message = "Hello world." c.BinderHub.banner_message = 'This is headline news.' From dc9b84baca0e842679316fea301ea3bd15ad6f06 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 7 Apr 2023 15:11:13 +0300 Subject: [PATCH 10/48] Rename configs and change logic --- binderhub/app.py | 4 ++-- binderhub/builder.py | 28 +++++++++++++++++++--------- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/binderhub/app.py b/binderhub/app.py index f9676ca1c..2821424af 100644 --- a/binderhub/app.py +++ b/binderhub/app.py @@ -786,7 +786,7 @@ def _template_path_default(self): help="Origin to use when emitting events. Defaults to hostname of request when empty", ) - no_launch = Bool( + require_build_only = Bool( False, help="When enabled, the hub will no longer launch the image after the build", config=True, @@ -949,7 +949,7 @@ def initialize(self, *args, **kwargs): "auth_enabled": self.auth_enabled, "event_log": self.event_log, "normalized_origin": self.normalized_origin, - "no_launch": self.no_launch, + "require_build_only": self.require_build_only, } ) if self.auth_enabled: diff --git a/binderhub/builder.py b/binderhub/builder.py index 590e882ff..8fbbda7cf 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -408,17 +408,27 @@ async def get(self, provider_prefix, _unescaped_spec): else: image_found = True - no_launch = self.settings.get("no_launch", False) - no_launch_req_query_attr = self.get_query_argument(name="no-launch", default="") - # The request query attribute takes precedence over the traitlet config - if no_launch_req_query_attr: - no_launch = True - if no_launch: + require_build_only = self.settings.get("require_build_only", False) + build_only = False + if not require_build_only: + build_only_query_parameter = self.get_query_argument(name="build_only", default="") + if build_only_query_parameter.lower() == "true": + raise ValueError("Building but not launching is not permitted!") + else: + # Not setting a default will make the function raise an error + # if the `build_only` query parameter is missing from the request + build_only_query_parameter = self.get_query_argument(name="build_only") + if build_only_query_parameter.lower() != "true": + raise ValueError("The `build_only=true` query parameter is required!") + # If we're here, it means a build only deployment is required + build_only = True + + if build_only: await self.emit( { "phase": "info", "imageName": image_name, - "message": "Found no launch option. Image will not be launched after build.\n", + "message": "Build only was enabled. Image will not be launched after build.\n", } ) if image_found: @@ -430,7 +440,7 @@ async def get(self, provider_prefix, _unescaped_spec): } ) with LAUNCHES_INPROGRESS.track_inprogress(): - if no_launch: + if build_only: await self.emit( { "phase": "built", @@ -581,7 +591,7 @@ def _check_result(future): ) BUILD_COUNT.labels(status="success", **self.repo_metric_labels).inc() with LAUNCHES_INPROGRESS.track_inprogress(): - if no_launch: + if build_only: await self.emit( { "phase": "ready", From 835a43b6c31c39d8595c55a6c1aa53d894c19ddc Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 7 Apr 2023 15:27:49 +0300 Subject: [PATCH 11/48] Change error type --- binderhub/builder.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index 8fbbda7cf..c3e9b2591 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -19,7 +19,7 @@ from tornado.iostream import StreamClosedError from tornado.log import app_log from tornado.queues import Queue -from tornado.web import Finish, authenticated +from tornado.web import Finish, authenticated, HTTPError from .base import BaseHandler from .build import ProgressEvent @@ -413,13 +413,13 @@ async def get(self, provider_prefix, _unescaped_spec): if not require_build_only: build_only_query_parameter = self.get_query_argument(name="build_only", default="") if build_only_query_parameter.lower() == "true": - raise ValueError("Building but not launching is not permitted!") + raise HTTPError(log_message="Building but not launching is not permitted!") else: # Not setting a default will make the function raise an error # if the `build_only` query parameter is missing from the request build_only_query_parameter = self.get_query_argument(name="build_only") if build_only_query_parameter.lower() != "true": - raise ValueError("The `build_only=true` query parameter is required!") + raise HTTPError(log_message="The `build_only=true` query parameter is required!") # If we're here, it means a build only deployment is required build_only = True From fbdf0c6eafeb814af5af44a938af7da327d09f6b Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 7 Apr 2023 15:28:00 +0300 Subject: [PATCH 12/48] Update examples --- examples/binder-api.py | 14 ++++++++------ .../local-binder-mocked-hub/binderhub_config.py | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/examples/binder-api.py b/examples/binder-api.py index a2cd1cb44..4b1037547 100644 --- a/examples/binder-api.py +++ b/examples/binder-api.py @@ -15,7 +15,7 @@ import requests -def build_binder(repo, ref, *, binder_url="https://mybinder.org", no_launch): +def build_binder(repo, ref, *, binder_url="https://mybinder.org", build_only): """Launch a binder Yields Binder's event-stream events (dicts) @@ -23,8 +23,8 @@ def build_binder(repo, ref, *, binder_url="https://mybinder.org", no_launch): print(f"Building binder for {repo}@{ref}") url = binder_url + f"/build/gh/{repo}/{ref}" params = {} - if no_launch: - params = {"no-launch": "True"} + if build_only: + params = {"build_only": "true"} r = requests.get(url, stream=True, params=params) r.raise_for_status() @@ -39,7 +39,7 @@ def build_binder(repo, ref, *, binder_url="https://mybinder.org", no_launch): parser.add_argument("repo", type=str, help="The GitHub repo to build") parser.add_argument("--ref", default="HEAD", help="The ref of the repo to build") parser.add_argument( - "--no-launch", + "--build-only", action="store_true", help="When passed, the image will not be launched after build", ) @@ -57,7 +57,7 @@ def build_binder(repo, ref, *, binder_url="https://mybinder.org", no_launch): opts = parser.parse_args() for evt in build_binder( - opts.repo, ref=opts.ref, binder_url=opts.binder, no_launch=opts.no_launch + opts.repo, ref=opts.ref, binder_url=opts.binder, build_only=opts.build_only ): if "message" in evt: print( @@ -67,7 +67,9 @@ def build_binder(repo, ref, *, binder_url="https://mybinder.org", no_launch): ) ) if evt.get("phase") == "ready": - if opts.filepath: + if opts.build_only: + break + elif opts.filepath: url = "{url}notebooks/{filepath}?token={token}".format( **evt, filepath=opts.filepath ) diff --git a/testing/local-binder-mocked-hub/binderhub_config.py b/testing/local-binder-mocked-hub/binderhub_config.py index d5d341723..58ab8b6d3 100644 --- a/testing/local-binder-mocked-hub/binderhub_config.py +++ b/testing/local-binder-mocked-hub/binderhub_config.py @@ -17,7 +17,7 @@ c.BinderHub.repo_providers = {"gh": FakeProvider} c.BinderHub.build_class = FakeBuild # Uncomment the following line to enable BinderHub to not launch the image after build -# c.BinderHub.no_launch = True +# c.BinderHub.require_build_only = True c.BinderHub.about_message = "Hello world." c.BinderHub.banner_message = 'This is headline news.' From 3c310e92aa3a12b003258fd71b23b404c22c2a1e Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 7 Apr 2023 15:41:27 +0300 Subject: [PATCH 13/48] Update initial test --- binderhub/builder.py | 14 +++++++++---- binderhub/tests/test_build.py | 38 ++++++++++++----------------------- 2 files changed, 23 insertions(+), 29 deletions(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index c3e9b2591..bee2eb3e3 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -19,7 +19,7 @@ from tornado.iostream import StreamClosedError from tornado.log import app_log from tornado.queues import Queue -from tornado.web import Finish, authenticated, HTTPError +from tornado.web import Finish, HTTPError, authenticated from .base import BaseHandler from .build import ProgressEvent @@ -411,15 +411,21 @@ async def get(self, provider_prefix, _unescaped_spec): require_build_only = self.settings.get("require_build_only", False) build_only = False if not require_build_only: - build_only_query_parameter = self.get_query_argument(name="build_only", default="") + build_only_query_parameter = self.get_query_argument( + name="build_only", default="" + ) if build_only_query_parameter.lower() == "true": - raise HTTPError(log_message="Building but not launching is not permitted!") + raise HTTPError( + log_message="Building but not launching is not permitted!" + ) else: # Not setting a default will make the function raise an error # if the `build_only` query parameter is missing from the request build_only_query_parameter = self.get_query_argument(name="build_only") if build_only_query_parameter.lower() != "true": - raise HTTPError(log_message="The `build_only=true` query parameter is required!") + raise HTTPError( + log_message="The `build_only=true` query parameter is required!" + ) # If we're here, it means a build only deployment is required build_only = True diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index f9b6ae6f3..aa4e282e6 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -12,6 +12,7 @@ from kubernetes import client from tornado.httputil import url_concat from tornado.queues import Queue +from tornado.web import HTTPError from binderhub.build import KubernetesBuildExecutor, ProgressEvent from binderhub.build_local import LocalRepo2dockerBuild, ProcessTerminated, _execute_cmd @@ -122,46 +123,33 @@ async def test_build_fail(app, needs_build, needs_launch, always_build, pytestco @pytest.mark.asyncio(timeout=120) @pytest.mark.remote -async def test_build_no_launch(app): +async def test_build_no_launch_failing(app): """ - Test build endpoint with no launch option active, passed as a request query param. + Test build endpoint with build_only launch option active, + passed as a request query param, + but `require_build_only = False`. """ slug = "gh/binderhub-ci-repos/cached-minimal-dockerfile/HEAD" build_url = f"{app.url}/build/{slug}" params = {} - params = {"no-launch": "True"} + params = {"build_only": "true"} r = await async_requests.get(build_url, stream=True, params=params) r.raise_for_status() - - events = [] - launch_events = 0 + failed_events = 0 async for line in async_requests.iter_lines(r): line = line.decode("utf8", "replace") if line.startswith("data:"): event = json.loads(line.split(":", 1)[1]) - events.append(event) - assert "message" in event - sys.stdout.write(f"{event.get('phase', '')}: {event['message']}") - if event.get("phase") == "info": + assert event.get("phase") not in ("launching", "ready") + if event.get("phase") == "failed": + failed_events += 1 assert ( - "Found no launch option. Image will not be launched after build" - in event["message"] + "Building but not launching is not permitted!" in event["message"] ) - # This is the signal that the image was built. - # Check the message for clue that the image won't be launched and - # break out of the loop now because BinderHub keeps the connection open - # for many seconds after to avoid "reconnects" from slow clients - if event.get("phase") == "ready": - assert "Image won't be launched" in event["message"] - r.close() break - if event.get("phase") == "launching": - launch_events += 1 + r.close() - assert launch_events == 0 - final = events[-1] - assert "phase" in final - assert final["phase"] == "ready" + assert failed_events > 0, "Should have seen phase 'failed'" def _list_image_builder_pods_mock(): From 27c294472ccf07ab9d92a21475965d1d7372f6d1 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 7 Apr 2023 15:59:55 +0300 Subject: [PATCH 14/48] Remove unused import --- binderhub/tests/test_build.py | 1 - 1 file changed, 1 deletion(-) diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index aa4e282e6..1991e09b4 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -12,7 +12,6 @@ from kubernetes import client from tornado.httputil import url_concat from tornado.queues import Queue -from tornado.web import HTTPError from binderhub.build import KubernetesBuildExecutor, ProgressEvent from binderhub.build_local import LocalRepo2dockerBuild, ProcessTerminated, _execute_cmd From 0aff6a8ba06a6f822103f95c1127c437eb20a604 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Thu, 20 Apr 2023 14:23:16 +0300 Subject: [PATCH 15/48] Update messages and logic to preserve old ones but also be explicit about new options --- binderhub/builder.py | 138 +++++++++++++++++++--------------- binderhub/tests/test_build.py | 1 - examples/binder-api.py | 2 +- 3 files changed, 78 insertions(+), 63 deletions(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index bee2eb3e3..5f0dd354a 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -408,71 +408,88 @@ async def get(self, provider_prefix, _unescaped_spec): else: image_found = True + # Get the value of the `require_build_only` traitlet require_build_only = self.settings.get("require_build_only", False) - build_only = False + # Get the value of the `build_only` query parameter if present + build_only_query_parameter = self.get_query_argument( + name="build_only", default="" + ) + log_message_eval_table = ( + "Table for evaluating whether or not the image will be launched after build" + "based on the values of the `require_build_only` traitlet and the `build_only` query parameter." + """ + | `require_build_only` trait | `build_only` query param | Outcome + ------------------------------------------------------------------------------------------------ + | false | missing | OK, image will be launched after build + | false | false | OK, image will be launched after build + | false | true | ERROR, building but not launching is not permitted when require_build_only == False + | true | missing | ERROR, query parameter must be explicitly set to true when require_build_only == True + | true | false | ERROR, query parameter must be explicitly set to true when require_build_only == True + | true | true | OK, image won't be launched after build + """ + ) + # Whether or not the image should only be built and not launched + build_only_outcome = False if not require_build_only: - build_only_query_parameter = self.get_query_argument( - name="build_only", default="" - ) if build_only_query_parameter.lower() == "true": raise HTTPError( - log_message="Building but not launching is not permitted!" + log_message="Building but not launching is not permitted when " + "traitlet `require_build_only == False` and query parameter `build_only == True`! " + "See the table below for more info.\n\n" + log_message_eval_table ) else: - # Not setting a default will make the function raise an error - # if the `build_only` query parameter is missing from the request - build_only_query_parameter = self.get_query_argument(name="build_only") + # Raise an error if the `build_only` query parameter is anything but `(T)true` if build_only_query_parameter.lower() != "true": raise HTTPError( - log_message="The `build_only=true` query parameter is required!" + log_message="The `build_only=true` query parameter is required when traitlet `require_build_only == False`!" + "See the table below for more info.\n\n" + log_message_eval_table ) # If we're here, it means a build only deployment is required - build_only = True - - if build_only: + build_only_outcome = True await self.emit( { "phase": "info", "imageName": image_name, - "message": "Build only was enabled. Image will not be launched after build.\n", + "message": "Built image will not be launched\n", } ) + if image_found: - await self.emit( - { - "phase": "built", - "imageName": image_name, - "message": "Found built image\n", - } - ) - with LAUNCHES_INPROGRESS.track_inprogress(): - if build_only: - await self.emit( - { - "phase": "built", - "imageName": image_name, - "message": "Image won't be launched\n", - } - ) - else: + if build_only_outcome: + await self.emit( + { + "phase": "ready", + "imageName": image_name, + "message": "Done! Found built image\n", + } + ) + else: + await self.emit( + { + "phase": "built", + "imageName": image_name, + "message": "Found built image, launching...\n", + } + ) + with LAUNCHES_INPROGRESS.track_inprogress(): try: await self.launch(provider) except LaunchQuotaExceeded: return - self.event_log.emit( - "binderhub.jupyter.org/launch", - 5, - { - "provider": provider.name, - "spec": spec, - "ref": ref, - "status": "success", - "build_token": self._have_build_token, - "origin": self.settings["normalized_origin"] - if self.settings["normalized_origin"] - else self.request.host, - }, - ) + self.event_log.emit( + "binderhub.jupyter.org/launch", + 5, + { + "provider": provider.name, + "spec": spec, + "ref": ref, + "status": "success", + "build_token": self._have_build_token, + "origin": self.settings["normalized_origin"] + if self.settings["normalized_origin"] + else self.request.host, + }, + ) return # Don't allow builds when quota is exceeded @@ -551,9 +568,14 @@ def _check_result(future): # nothing to do, just waiting continue elif progress.payload == ProgressEvent.BuildStatus.BUILT: + if build_only_outcome: + message = "Done! Image built\n" + phase = "ready" + else: + message = "Built image, launching...\n" event = { "phase": phase, - "message": "Done! Image built.\n", + "message": message, "imageName": image_name, } done = True @@ -590,22 +612,16 @@ def _check_result(future): await self.emit(event) - # Launch after building an image - if not failed: - BUILD_TIME.labels(status="success").observe( - time.perf_counter() - build_starttime - ) - BUILD_COUNT.labels(status="success", **self.repo_metric_labels).inc() - with LAUNCHES_INPROGRESS.track_inprogress(): - if build_only: - await self.emit( - { - "phase": "ready", - "imageName": image_name, - "message": "Image won't be launched\n", - } - ) - else: + if build_only_outcome: + return + else: + # Launch after building an image + if not failed: + BUILD_TIME.labels(status="success").observe( + time.perf_counter() - build_starttime + ) + BUILD_COUNT.labels(status="success", **self.repo_metric_labels).inc() + with LAUNCHES_INPROGRESS.track_inprogress(): await self.launch(provider) self.event_log.emit( "binderhub.jupyter.org/launch", diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index 1991e09b4..0253c9da5 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -130,7 +130,6 @@ async def test_build_no_launch_failing(app): """ slug = "gh/binderhub-ci-repos/cached-minimal-dockerfile/HEAD" build_url = f"{app.url}/build/{slug}" - params = {} params = {"build_only": "true"} r = await async_requests.get(build_url, stream=True, params=params) r.raise_for_status() diff --git a/examples/binder-api.py b/examples/binder-api.py index 4b1037547..d64510dbb 100644 --- a/examples/binder-api.py +++ b/examples/binder-api.py @@ -48,7 +48,7 @@ def build_binder(repo, ref, *, binder_url="https://mybinder.org", build_only): file_or_url.add_argument("--urlpath", type=str, help="The url to open, if any.") parser.add_argument( "--binder", - default="http://localhost:8585", + default="https://mybinder.org", help=""" The URL of the binder instance to use. Use `http://localhost:8585` if you are doing local testing. From e67a84bbcea5df9201fde3d282ca76a893d820c7 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Thu, 20 Apr 2023 14:30:06 +0300 Subject: [PATCH 16/48] Update one more message --- binderhub/builder.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index 5f0dd354a..1de56dbaf 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -450,7 +450,8 @@ async def get(self, provider_prefix, _unescaped_spec): { "phase": "info", "imageName": image_name, - "message": "Built image will not be launched\n", + "message": "Both require_build_only traitlet, and the query parameter build_only are True, " + "so built image will not be launched\n", } ) From b80eaf759b0450ccef0de7e1e0f5d9e50bf8a01b Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Thu, 20 Apr 2023 16:49:14 +0300 Subject: [PATCH 17/48] Add more test for failing cases --- binderhub/builder.py | 4 ++-- binderhub/tests/conftest.py | 24 +++++++++++++++---- binderhub/tests/test_build.py | 44 ++++++++++++++++++++++++++++------- 3 files changed, 58 insertions(+), 14 deletions(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index 1de56dbaf..cde95165d 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -434,14 +434,14 @@ async def get(self, provider_prefix, _unescaped_spec): if build_only_query_parameter.lower() == "true": raise HTTPError( log_message="Building but not launching is not permitted when " - "traitlet `require_build_only == False` and query parameter `build_only == True`! " + "traitlet `require_build_only` is false and query parameter `build_only` is true! " "See the table below for more info.\n\n" + log_message_eval_table ) else: # Raise an error if the `build_only` query parameter is anything but `(T)true` if build_only_query_parameter.lower() != "true": raise HTTPError( - log_message="The `build_only=true` query parameter is required when traitlet `require_build_only == False`!" + log_message="The `build_only=true` query parameter is required when traitlet `require_build_only` is false! " "See the table below for more info.\n\n" + log_message_eval_table ) # If we're here, it means a build only deployment is required diff --git a/binderhub/tests/conftest.py b/binderhub/tests/conftest.py index 63cd6677f..8607a6a99 100644 --- a/binderhub/tests/conftest.py +++ b/binderhub/tests/conftest.py @@ -30,6 +30,10 @@ root, "testing/local-binder-k8s-hub/binderhub_config_auth_additions.py" ) +binderhub_config_build_only_addition_path = os.path.join( + root, "testing/local-binder-k8s-hub/binderhub_config_build_only_addition.py" +) + # These are automatically determined K8S_AVAILABLE = False K8S_NAMESPACE = None @@ -252,10 +256,16 @@ def app(request, io_loop, _binderhub_config): app._configured_bhub = BinderHub(config=_binderhub_config) return app - if hasattr(request, "param") and request.param is True: - # load conf for auth test - cfg = PyFileConfigLoader(binderhub_config_auth_additions_path).load_config() - _binderhub_config.merge(cfg) + if hasattr(request, "param"): + if request.param is True: + # load conf for auth test + cfg = PyFileConfigLoader(binderhub_config_auth_additions_path).load_config() + _binderhub_config.merge(cfg) + elif request.param == "require_build_only_app": + cfg = PyFileConfigLoader( + binderhub_config_build_only_addition_path + ).load_config() + _binderhub_config.merge(cfg) bhub = BinderHub.instance(config=_binderhub_config) bhub.initialize([]) bhub.start(run_loop=False) @@ -273,6 +283,12 @@ def cleanup(): return bhub +@pytest.fixture +def build_only_configured_app(app): + with mock.patch.dict(app.tornado_settings, {"require_build_only": False}): + yield app + + def cleanup_pods(labels): """Cleanup pods in current namespace that match the given labels""" kube = kubernetes.client.CoreV1Api() diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index 0253c9da5..12c11458e 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -122,15 +122,45 @@ async def test_build_fail(app, needs_build, needs_launch, always_build, pytestco @pytest.mark.asyncio(timeout=120) @pytest.mark.remote -async def test_build_no_launch_failing(app): +@pytest.mark.parametrize( + "app,build_only,expected_error_msg", + [ + ("default_app", True, "Building but not launching is not permitted"), + ( + "require_build_only_app", + False, + "The `build_only=true` query parameter is required", + ), + ( + "require_build_only_app", + "", + "The `build_only=true` query parameter is required", + ), + ], + indirect=[ + "app" + ], # send param "require_build_only_app" to app fixture, so that it loads `require_build_only` configuration +) +async def test_build_no_launch_fail(app, build_only, expected_error_msg): """ - Test build endpoint with build_only launch option active, - passed as a request query param, - but `require_build_only = False`. + Test the scenarios that are expected to fail when setting configs for building but no launching. + + Table for evaluating whether or not the image will be launched after build based on the values of + the `require_build_only` traitlet and the `build_only` query parameter. + + | `require_build_only` trait | `build_only` query param | Outcome + ------------------------------------------------------------------------------------------------ + | false | missing | OK, image will be launched after build + | false | false | OK, image will be launched after build + | false | true | ERROR, building but not launching is not permitted when require_build_only == False + | true | missing | ERROR, query parameter must be explicitly set to true when require_build_only == True + | true | false | ERROR, query parameter must be explicitly set to true when require_build_only == True + | true | true | OK, image won't be launched after build """ + slug = "gh/binderhub-ci-repos/cached-minimal-dockerfile/HEAD" build_url = f"{app.url}/build/{slug}" - params = {"build_only": "true"} + params = {"build_only": build_only} r = await async_requests.get(build_url, stream=True, params=params) r.raise_for_status() failed_events = 0 @@ -141,9 +171,7 @@ async def test_build_no_launch_failing(app): assert event.get("phase") not in ("launching", "ready") if event.get("phase") == "failed": failed_events += 1 - assert ( - "Building but not launching is not permitted!" in event["message"] - ) + assert expected_error_msg in event["message"] break r.close() From 97cabe3860c59fb0038904f8cac2c3d7e7e48ead Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Thu, 20 Apr 2023 17:07:15 +0300 Subject: [PATCH 18/48] Add missing config --- .../local-binder-k8s-hub/binderhub_config_build_only_addition.py | 1 + 1 file changed, 1 insertion(+) create mode 100644 testing/local-binder-k8s-hub/binderhub_config_build_only_addition.py diff --git a/testing/local-binder-k8s-hub/binderhub_config_build_only_addition.py b/testing/local-binder-k8s-hub/binderhub_config_build_only_addition.py new file mode 100644 index 000000000..23ec4af23 --- /dev/null +++ b/testing/local-binder-k8s-hub/binderhub_config_build_only_addition.py @@ -0,0 +1 @@ +c.BinderHub.require_build_only = True From 0edd7826b8aa27fb2385f5fb77a242e28dd964dd Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Thu, 20 Apr 2023 17:39:31 +0300 Subject: [PATCH 19/48] Test with build_only true --- binderhub/builder.py | 2 - binderhub/tests/test_build.py | 73 +++++++++++++++++++++++++++++++++-- 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index cde95165d..d52ddc857 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -560,7 +560,6 @@ def _check_result(future): while not done: progress = await q.get() - # FIXME: If pod goes into an unrecoverable stage, such as ImagePullBackoff or # whatever, we should fail properly. if progress.kind == ProgressEvent.Kind.BUILD_STATUS_CHANGE: @@ -610,7 +609,6 @@ def _check_result(future): BUILD_COUNT.labels( status="failure", **self.repo_metric_labels ).inc() - await self.emit(event) if build_only_outcome: diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index 12c11458e..0692ff735 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -96,9 +96,73 @@ async def test_build(app, needs_build, needs_launch, always_build, slug, pytestc assert r.url.startswith(final["url"]) +@pytest.mark.remote +@pytest.mark.parametrize( + "app,slug", + [ + ( + "require_build_only_app", + "git/{}/596b52f10efb0c9befc0c4ae850cc5175297d71c".format( + quote( + "https://github.com/binderhub-ci-repos/cached-minimal-dockerfile", + safe="", + ) + ), + ), + ( + "require_build_only_app", + "gh/binderhub-ci-repos/cached-minimal-dockerfile/HEAD", + ), + ], + indirect=[ + "app" + ], # send param "require_build_only_app" to app fixture, so that it loads `require_build_only` configuration +) +async def test_build_only(app, slug): + """ + Test build a repo that is very quick and easy to build. + """ + build_url = f"{app.url}/build/{slug}" + r = await async_requests.get(build_url, stream=True, params={"build_only": True}) + r.raise_for_status() + events = [] + launch_events = 0 + async for line in async_requests.iter_lines(r): + line = line.decode("utf8", "replace") + if line.startswith("data:"): + event = json.loads(line.split(":", 1)[1]) + events.append(event) + assert "message" in event + sys.stdout.write(f"{event.get('phase', '')}: {event['message']}") + # this is the signal that everything is ready, pod is launched + # and server is up inside the pod. Break out of the loop now + # because BinderHub keeps the connection open for many seconds + # after to avoid "reconnects" from slow clients + if event.get("phase") == "ready": + r.close() + break + if event.get("phase") == "info": + assert ( + "Both require_build_only traitlet, and the query parameter build_only are True" + in event["message"] + ) + if event.get("phase") == "launching" and not event["message"].startswith( + ("Launching server...", "Launch attempt ") + ): + # skip standard launching events of builder + # we are interested in launching events from spawner + launch_events += 1 + + assert launch_events == 0 + final = events[-1] + assert "phase" in final + assert final["phase"] == "ready" + print(final["url"]) + + @pytest.mark.asyncio(timeout=120) @pytest.mark.remote -async def test_build_fail(app, needs_build, needs_launch, always_build, pytestconfig): +async def test_build_fail(app): """ Test build a repo that should fail immediately. """ @@ -141,7 +205,7 @@ async def test_build_fail(app, needs_build, needs_launch, always_build, pytestco "app" ], # send param "require_build_only_app" to app fixture, so that it loads `require_build_only` configuration ) -async def test_build_no_launch_fail(app, build_only, expected_error_msg): +async def test_build_only_fail(app, build_only, expected_error_msg): """ Test the scenarios that are expected to fail when setting configs for building but no launching. @@ -160,8 +224,9 @@ async def test_build_no_launch_fail(app, build_only, expected_error_msg): slug = "gh/binderhub-ci-repos/cached-minimal-dockerfile/HEAD" build_url = f"{app.url}/build/{slug}" - params = {"build_only": build_only} - r = await async_requests.get(build_url, stream=True, params=params) + r = await async_requests.get( + build_url, stream=True, params={"build_only": build_only} + ) r.raise_for_status() failed_events = 0 async for line in async_requests.iter_lines(r): From 7f5156bd6354f99b022c8911a37400f655e7675f Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 21 Apr 2023 11:57:55 +0300 Subject: [PATCH 20/48] Allow for build_only configured app fixture in all kind of tests --- binderhub/builder.py | 2 +- binderhub/tests/conftest.py | 27 +++++++++++---------------- binderhub/tests/test_build.py | 18 ++---------------- 3 files changed, 14 insertions(+), 33 deletions(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index d52ddc857..a3862159b 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -450,7 +450,7 @@ async def get(self, provider_prefix, _unescaped_spec): { "phase": "info", "imageName": image_name, - "message": "Both require_build_only traitlet, and the query parameter build_only are True, " + "message": "Both require_build_only traitlet, and the query parameter build_only are true, " "so built image will not be launched\n", } ) diff --git a/binderhub/tests/conftest.py b/binderhub/tests/conftest.py index 8607a6a99..6442fb8b2 100644 --- a/binderhub/tests/conftest.py +++ b/binderhub/tests/conftest.py @@ -232,6 +232,12 @@ def app(request, io_loop, _binderhub_config): app.url will contain the base URL of binderhub. """ + if hasattr(request, "param") and request.param == "require_build_only_app": + cfg = PyFileConfigLoader( + binderhub_config_build_only_addition_path + ).load_config() + _binderhub_config.merge(cfg) + if REMOTE_BINDER: app = RemoteBinderHub() # wait for the remote binder to be up @@ -256,16 +262,11 @@ def app(request, io_loop, _binderhub_config): app._configured_bhub = BinderHub(config=_binderhub_config) return app - if hasattr(request, "param"): - if request.param is True: - # load conf for auth test - cfg = PyFileConfigLoader(binderhub_config_auth_additions_path).load_config() - _binderhub_config.merge(cfg) - elif request.param == "require_build_only_app": - cfg = PyFileConfigLoader( - binderhub_config_build_only_addition_path - ).load_config() - _binderhub_config.merge(cfg) + if hasattr(request, "param") and request.param is True: + # load conf for auth test + cfg = PyFileConfigLoader(binderhub_config_auth_additions_path).load_config() + _binderhub_config.merge(cfg) + bhub = BinderHub.instance(config=_binderhub_config) bhub.initialize([]) bhub.start(run_loop=False) @@ -283,12 +284,6 @@ def cleanup(): return bhub -@pytest.fixture -def build_only_configured_app(app): - with mock.patch.dict(app.tornado_settings, {"require_build_only": False}): - yield app - - def cleanup_pods(labels): """Cleanup pods in current namespace that match the given labels""" kube = kubernetes.client.CoreV1Api() diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index 0692ff735..c211426a0 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -49,7 +49,7 @@ ], ) @pytest.mark.remote -async def test_build(app, needs_build, needs_launch, always_build, slug, pytestconfig): +async def test_build(app, slug, pytestconfig): """ Test build a repo that is very quick and easy to build. """ @@ -100,15 +100,6 @@ async def test_build(app, needs_build, needs_launch, always_build, slug, pytestc @pytest.mark.parametrize( "app,slug", [ - ( - "require_build_only_app", - "git/{}/596b52f10efb0c9befc0c4ae850cc5175297d71c".format( - quote( - "https://github.com/binderhub-ci-repos/cached-minimal-dockerfile", - safe="", - ) - ), - ), ( "require_build_only_app", "gh/binderhub-ci-repos/cached-minimal-dockerfile/HEAD", @@ -134,16 +125,12 @@ async def test_build_only(app, slug): events.append(event) assert "message" in event sys.stdout.write(f"{event.get('phase', '')}: {event['message']}") - # this is the signal that everything is ready, pod is launched - # and server is up inside the pod. Break out of the loop now - # because BinderHub keeps the connection open for many seconds - # after to avoid "reconnects" from slow clients if event.get("phase") == "ready": r.close() break if event.get("phase") == "info": assert ( - "Both require_build_only traitlet, and the query parameter build_only are True" + "Both require_build_only traitlet, and the query parameter build_only are true" in event["message"] ) if event.get("phase") == "launching" and not event["message"].startswith( @@ -157,7 +144,6 @@ async def test_build_only(app, slug): final = events[-1] assert "phase" in final assert final["phase"] == "ready" - print(final["url"]) @pytest.mark.asyncio(timeout=120) From dabfb6d32d12a47f9d0a29e8e6601597ac753d8c Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 21 Apr 2023 13:22:27 +0300 Subject: [PATCH 21/48] Do not mark tests remote as we cannot configure binderhub with the desired config --- binderhub/builder.py | 4 ++-- binderhub/tests/conftest.py | 20 +++++++++++--------- binderhub/tests/test_build.py | 3 +-- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index a3862159b..fda687683 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -411,8 +411,8 @@ async def get(self, provider_prefix, _unescaped_spec): # Get the value of the `require_build_only` traitlet require_build_only = self.settings.get("require_build_only", False) # Get the value of the `build_only` query parameter if present - build_only_query_parameter = self.get_query_argument( - name="build_only", default="" + build_only_query_parameter = str( + self.get_query_argument(name="build_only", default="") ) log_message_eval_table = ( "Table for evaluating whether or not the image will be launched after build" diff --git a/binderhub/tests/conftest.py b/binderhub/tests/conftest.py index 6442fb8b2..5c311f3e7 100644 --- a/binderhub/tests/conftest.py +++ b/binderhub/tests/conftest.py @@ -232,11 +232,6 @@ def app(request, io_loop, _binderhub_config): app.url will contain the base URL of binderhub. """ - if hasattr(request, "param") and request.param == "require_build_only_app": - cfg = PyFileConfigLoader( - binderhub_config_build_only_addition_path - ).load_config() - _binderhub_config.merge(cfg) if REMOTE_BINDER: app = RemoteBinderHub() @@ -262,10 +257,17 @@ def app(request, io_loop, _binderhub_config): app._configured_bhub = BinderHub(config=_binderhub_config) return app - if hasattr(request, "param") and request.param is True: - # load conf for auth test - cfg = PyFileConfigLoader(binderhub_config_auth_additions_path).load_config() - _binderhub_config.merge(cfg) + if hasattr(request, "param"): + if request.param is True: + # load conf for auth test + cfg = PyFileConfigLoader(binderhub_config_auth_additions_path).load_config() + _binderhub_config.merge(cfg) + elif request.param == "require_build_only_app": + # load conf that sets BinderHub.require_build_only = True + cfg = PyFileConfigLoader( + binderhub_config_build_only_addition_path + ).load_config() + _binderhub_config.merge(cfg) bhub = BinderHub.instance(config=_binderhub_config) bhub.initialize([]) diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index c211426a0..1c0afe1f6 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -96,7 +96,7 @@ async def test_build(app, slug, pytestconfig): assert r.url.startswith(final["url"]) -@pytest.mark.remote +@pytest.mark.asyncio(timeout=900) @pytest.mark.parametrize( "app,slug", [ @@ -171,7 +171,6 @@ async def test_build_fail(app): @pytest.mark.asyncio(timeout=120) -@pytest.mark.remote @pytest.mark.parametrize( "app,build_only,expected_error_msg", [ From 1f4db5fe5d2fa90264616415387d1d47fcf6c01a Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 21 Apr 2023 14:29:23 +0300 Subject: [PATCH 22/48] _binderhub_config fixture has a session scope --- binderhub/tests/conftest.py | 18 ++++++++++----- binderhub/tests/test_auth.py | 4 ++-- binderhub/tests/test_build.py | 22 +++++++++++-------- .../binderhub_config_build_only_addition.py | 1 - 4 files changed, 28 insertions(+), 17 deletions(-) delete mode 100644 testing/local-binder-k8s-hub/binderhub_config_build_only_addition.py diff --git a/binderhub/tests/conftest.py b/binderhub/tests/conftest.py index 5c311f3e7..ec5d13879 100644 --- a/binderhub/tests/conftest.py +++ b/binderhub/tests/conftest.py @@ -16,6 +16,7 @@ import requests from tornado.httpclient import AsyncHTTPClient from tornado.platform.asyncio import AsyncIOMainLoop +from traitlets.config import Config from traitlets.config.loader import PyFileConfigLoader from ..app import BinderHub @@ -257,17 +258,24 @@ def app(request, io_loop, _binderhub_config): app._configured_bhub = BinderHub(config=_binderhub_config) return app + build_only = False if hasattr(request, "param"): - if request.param is True: + if request.param == "app_with_auth_config": # load conf for auth test cfg = PyFileConfigLoader(binderhub_config_auth_additions_path).load_config() _binderhub_config.merge(cfg) - elif request.param == "require_build_only_app": + elif request.param == "app_with_require_build_only": # load conf that sets BinderHub.require_build_only = True - cfg = PyFileConfigLoader( - binderhub_config_build_only_addition_path - ).load_config() + cfg = Config({"BinderHub": {"require_build_only": True}}) _binderhub_config.merge(cfg) + build_only = True + + if not build_only: + # load conf that sets BinderHub.require_build_only = False + # otherwise because _binderhub_config has a session scope, + # any previous set of require_build_only to True will stick around + cfg = Config({"BinderHub": {"require_build_only": False}}) + _binderhub_config.merge(cfg) bhub = BinderHub.instance(config=_binderhub_config) bhub.initialize([]) diff --git a/binderhub/tests/test_auth.py b/binderhub/tests/test_auth.py index 7ef9e9a2b..9812f2224 100644 --- a/binderhub/tests/test_auth.py +++ b/binderhub/tests/test_auth.py @@ -23,13 +23,13 @@ def use_session(): @pytest.mark.parametrize( "app,path,authenticated", [ - (True, "/", True), # main page + ("app_with_auth_config", "/", True), # main page ( True, "/v2/gh/binderhub-ci-repos/requirements/d687a7f9e6946ab01ef2baa7bd6d5b73c6e904fd", True, ), - (True, "/metrics", False), + ("app_with_auth_config", "/metrics", False), ], indirect=[ "app" diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index 1c0afe1f6..559963244 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -98,21 +98,21 @@ async def test_build(app, slug, pytestconfig): @pytest.mark.asyncio(timeout=900) @pytest.mark.parametrize( - "app,slug", + "app,build_only", [ - ( - "require_build_only_app", - "gh/binderhub-ci-repos/cached-minimal-dockerfile/HEAD", - ), + ("app_with_require_build_only", True), + ("app_with_require_build_only", "true"), + ("app_with_require_build_only", "True"), ], indirect=[ "app" ], # send param "require_build_only_app" to app fixture, so that it loads `require_build_only` configuration ) -async def test_build_only(app, slug): +async def test_build_only(app, build_only): """ Test build a repo that is very quick and easy to build. """ + slug = "gh/binderhub-ci-repos/cached-minimal-dockerfile/HEAD" build_url = f"{app.url}/build/{slug}" r = await async_requests.get(build_url, stream=True, params={"build_only": True}) r.raise_for_status() @@ -174,17 +174,21 @@ async def test_build_fail(app): @pytest.mark.parametrize( "app,build_only,expected_error_msg", [ - ("default_app", True, "Building but not launching is not permitted"), ( - "require_build_only_app", + "app_with_require_build_only", False, "The `build_only=true` query parameter is required", ), ( - "require_build_only_app", + "app_with_require_build_only", "", "The `build_only=true` query parameter is required", ), + ( + "app_without_require_build_only", + True, + "Building but not launching is not permitted", + ), ], indirect=[ "app" diff --git a/testing/local-binder-k8s-hub/binderhub_config_build_only_addition.py b/testing/local-binder-k8s-hub/binderhub_config_build_only_addition.py deleted file mode 100644 index 23ec4af23..000000000 --- a/testing/local-binder-k8s-hub/binderhub_config_build_only_addition.py +++ /dev/null @@ -1 +0,0 @@ -c.BinderHub.require_build_only = True From 2f0e3cee78db91c62162860b0510086382f579bc Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 21 Apr 2023 14:59:56 +0300 Subject: [PATCH 23/48] Add more comments and do some cleanup --- binderhub/builder.py | 7 +++++-- binderhub/tests/conftest.py | 5 ----- binderhub/tests/test_auth.py | 2 +- binderhub/tests/test_build.py | 4 ++-- 4 files changed, 8 insertions(+), 10 deletions(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index fda687683..dadd48209 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -414,6 +414,9 @@ async def get(self, provider_prefix, _unescaped_spec): build_only_query_parameter = str( self.get_query_argument(name="build_only", default="") ) + # Describe how the traitlet and the query parameter need to be set together + # in order to get a build only behavior. + # Add this info table each time an unexpected (incorrect) config is observed. log_message_eval_table = ( "Table for evaluating whether or not the image will be launched after build" "based on the values of the `require_build_only` traitlet and the `build_only` query parameter." @@ -450,8 +453,8 @@ async def get(self, provider_prefix, _unescaped_spec): { "phase": "info", "imageName": image_name, - "message": "Both require_build_only traitlet, and the query parameter build_only are true, " - "so built image will not be launched\n", + "message": "Both `require_build_only` traitlet, and the query parameter `build_only` are true, " + "so the built image will not be launched\n", } ) diff --git a/binderhub/tests/conftest.py b/binderhub/tests/conftest.py index ec5d13879..c9361faed 100644 --- a/binderhub/tests/conftest.py +++ b/binderhub/tests/conftest.py @@ -31,10 +31,6 @@ root, "testing/local-binder-k8s-hub/binderhub_config_auth_additions.py" ) -binderhub_config_build_only_addition_path = os.path.join( - root, "testing/local-binder-k8s-hub/binderhub_config_build_only_addition.py" -) - # These are automatically determined K8S_AVAILABLE = False K8S_NAMESPACE = None @@ -233,7 +229,6 @@ def app(request, io_loop, _binderhub_config): app.url will contain the base URL of binderhub. """ - if REMOTE_BINDER: app = RemoteBinderHub() # wait for the remote binder to be up diff --git a/binderhub/tests/test_auth.py b/binderhub/tests/test_auth.py index 9812f2224..2780df0bb 100644 --- a/binderhub/tests/test_auth.py +++ b/binderhub/tests/test_auth.py @@ -33,7 +33,7 @@ def use_session(): ], indirect=[ "app" - ], # send param True to app fixture, so that it loads authentication configuration + ], # send param "app_with_auth_config" to app fixture, so that it loads authentication configuration ) @pytest.mark.auth async def test_auth(app, path, authenticated, use_session): diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index 559963244..0abbac6f4 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -114,7 +114,7 @@ async def test_build_only(app, build_only): """ slug = "gh/binderhub-ci-repos/cached-minimal-dockerfile/HEAD" build_url = f"{app.url}/build/{slug}" - r = await async_requests.get(build_url, stream=True, params={"build_only": True}) + r = await async_requests.get(build_url, stream=True, params={"build_only": build_only}) r.raise_for_status() events = [] launch_events = 0 @@ -130,7 +130,7 @@ async def test_build_only(app, build_only): break if event.get("phase") == "info": assert ( - "Both require_build_only traitlet, and the query parameter build_only are true" + "Both `require_build_only` traitlet, and the query parameter `build_only` are true" in event["message"] ) if event.get("phase") == "launching" and not event["message"].startswith( From 09da08d3c2c7cbe125a130abea35b965c487b00c Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 21 Apr 2023 12:00:15 +0000 Subject: [PATCH 24/48] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- binderhub/tests/test_build.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index 0abbac6f4..e42c5b182 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -114,7 +114,9 @@ async def test_build_only(app, build_only): """ slug = "gh/binderhub-ci-repos/cached-minimal-dockerfile/HEAD" build_url = f"{app.url}/build/{slug}" - r = await async_requests.get(build_url, stream=True, params={"build_only": build_only}) + r = await async_requests.get( + build_url, stream=True, params={"build_only": build_only} + ) r.raise_for_status() events = [] launch_events = 0 From 3a43d8e55943a2d0698b700ec19a8ed59124ee1d Mon Sep 17 00:00:00 2001 From: Georgiana Date: Mon, 24 Apr 2023 15:16:24 +0300 Subject: [PATCH 25/48] Only test one situation as running this test is toily Co-authored-by: Erik Sundell --- binderhub/tests/test_build.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index e42c5b182..418ca7b5c 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -100,8 +100,6 @@ async def test_build(app, slug, pytestconfig): @pytest.mark.parametrize( "app,build_only", [ - ("app_with_require_build_only", True), - ("app_with_require_build_only", "true"), ("app_with_require_build_only", "True"), ], indirect=[ From ff0670c30238ad5aba452e241367d4a0fd54b30f Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Mon, 24 Apr 2023 15:30:25 +0300 Subject: [PATCH 26/48] Put the fixtures back into the tests as they're needed --- binderhub/tests/test_build.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index 418ca7b5c..c30bcb7f1 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -49,7 +49,7 @@ ], ) @pytest.mark.remote -async def test_build(app, slug, pytestconfig): +async def test_build(app, needs_build, needs_launch, always_build, slug, pytestconfig): """ Test build a repo that is very quick and easy to build. """ @@ -148,7 +148,7 @@ async def test_build_only(app, build_only): @pytest.mark.asyncio(timeout=120) @pytest.mark.remote -async def test_build_fail(app): +async def test_build_fail(app, needs_build, needs_launch, always_build): """ Test build a repo that should fail immediately. """ From fc84a664741379fe347586a9b3a3a4997936d6cc Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Mon, 24 Apr 2023 15:40:41 +0300 Subject: [PATCH 27/48] Move logic about determining the build only action into its own function --- binderhub/builder.py | 90 +++++++++++++++++++++++--------------------- 1 file changed, 48 insertions(+), 42 deletions(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index dadd48209..2c7ff12e9 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -228,6 +228,52 @@ def set_default_headers(self): self.set_header("content-type", "text/event-stream") self.set_header("cache-control", "no-cache") + def _get_build_only_outcome(self): + # Get the value of the `require_build_only` traitlet + require_build_only = self.settings.get("require_build_only", False) + # Get the value of the `build_only` query parameter if present + build_only_query_parameter = str( + self.get_query_argument(name="build_only", default="") + ) + # Describe how the traitlet and the query parameter need to be set together + # in order to get a build only behavior. + # Add this info table each time an unexpected (incorrect) config is observed. + log_message_eval_table = ( + "Table for evaluating whether or not the image will be launched after build" + "based on the values of the `require_build_only` traitlet and the `build_only` query parameter." + """ + | `require_build_only` trait | `build_only` query param | Outcome + ------------------------------------------------------------------------------------------------ + | false | missing | OK, image will be launched after build + | false | false | OK, image will be launched after build + | false | true | ERROR, building but not launching is not permitted when require_build_only == False + | true | missing | ERROR, query parameter must be explicitly set to true when require_build_only == True + | true | false | ERROR, query parameter must be explicitly set to true when require_build_only == True + | true | true | OK, image won't be launched after build + """ + ) + # Whether or not the image should only be built and not launched + build_only_outcome = False + if not require_build_only: + if build_only_query_parameter.lower() == "true": + raise HTTPError( + log_message="Building but not launching is not permitted when " + "traitlet `require_build_only` is false and query parameter `build_only` is true! " + "See the table below for more info.\n\n" + log_message_eval_table + ) + else: + # Raise an error if the `build_only` query parameter is anything but `(T)true` + if build_only_query_parameter.lower() != "true": + raise HTTPError( + log_message="The `build_only=true` query parameter is required when traitlet `require_build_only` is false! " + "See the table below for more info.\n\n" + log_message_eval_table + ) + # If we're here, it means a build only deployment is required + build_only_outcome = True + + return build_only_outcome + + @authenticated async def get(self, provider_prefix, _unescaped_spec): """Get a built image for a given spec and repo provider. @@ -408,47 +454,8 @@ async def get(self, provider_prefix, _unescaped_spec): else: image_found = True - # Get the value of the `require_build_only` traitlet - require_build_only = self.settings.get("require_build_only", False) - # Get the value of the `build_only` query parameter if present - build_only_query_parameter = str( - self.get_query_argument(name="build_only", default="") - ) - # Describe how the traitlet and the query parameter need to be set together - # in order to get a build only behavior. - # Add this info table each time an unexpected (incorrect) config is observed. - log_message_eval_table = ( - "Table for evaluating whether or not the image will be launched after build" - "based on the values of the `require_build_only` traitlet and the `build_only` query parameter." - """ - | `require_build_only` trait | `build_only` query param | Outcome - ------------------------------------------------------------------------------------------------ - | false | missing | OK, image will be launched after build - | false | false | OK, image will be launched after build - | false | true | ERROR, building but not launching is not permitted when require_build_only == False - | true | missing | ERROR, query parameter must be explicitly set to true when require_build_only == True - | true | false | ERROR, query parameter must be explicitly set to true when require_build_only == True - | true | true | OK, image won't be launched after build - """ - ) - # Whether or not the image should only be built and not launched - build_only_outcome = False - if not require_build_only: - if build_only_query_parameter.lower() == "true": - raise HTTPError( - log_message="Building but not launching is not permitted when " - "traitlet `require_build_only` is false and query parameter `build_only` is true! " - "See the table below for more info.\n\n" + log_message_eval_table - ) - else: - # Raise an error if the `build_only` query parameter is anything but `(T)true` - if build_only_query_parameter.lower() != "true": - raise HTTPError( - log_message="The `build_only=true` query parameter is required when traitlet `require_build_only` is false! " - "See the table below for more info.\n\n" + log_message_eval_table - ) - # If we're here, it means a build only deployment is required - build_only_outcome = True + build_only_outcome = self._get_build_only_outcome() + if build_only_outcome: await self.emit( { "phase": "info", @@ -457,7 +464,6 @@ async def get(self, provider_prefix, _unescaped_spec): "so the built image will not be launched\n", } ) - if image_found: if build_only_outcome: await self.emit( From fb008c824f745656e64ab1e3df74e4f648e81883 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Mon, 24 Apr 2023 16:04:49 +0300 Subject: [PATCH 28/48] Move the eval table to the tratilet help string --- binderhub/app.py | 23 ++++++++++++++++++++++- binderhub/builder.py | 24 ++---------------------- 2 files changed, 24 insertions(+), 23 deletions(-) diff --git a/binderhub/app.py b/binderhub/app.py index 2821424af..e6df423cf 100644 --- a/binderhub/app.py +++ b/binderhub/app.py @@ -788,7 +788,28 @@ def _template_path_default(self): require_build_only = Bool( False, - help="When enabled, the hub will no longer launch the image after the build", + help="""When enabled, the hub will no longer launch the image after the build when combined with the appropriate + `build_only` request query parameter. + + Below it's the table for evaluating whether or not the image will be launched after build, + based on the values of the `require_build_only` traitlet and the `build_only` query parameter. + + +----------------------+--------------+---------------------------------------------------------------------------------------+ + | `require_build_only` | `build_only` | Outcome | + +======================+==============+=======================================================================================+ + | false | missing | OK, image will be launched after build | + +----------------------+--------------+---------------------------------------------------------------------------------------+ + | false | false | OK, image will be launched after build | + +----------------------+--------------+---------------------------------------------------------------------------------------+ + | false | true | ERROR, building but not launching is not permitted when require_build_only == False | + +----------------------+--------------+---------------------------------------------------------------------------------------+ + | true | missing | ERROR, query parameter must be explicitly set to true when require_build_only == True | + +----------------------+--------------+---------------------------------------------------------------------------------------+ + | true | false | ERROR, query parameter must be explicitly set to true when require_build_only == True | + +----------------------+--------------+---------------------------------------------------------------------------------------+ + | true | true | OK, image won't be launched after build | + +----------------------+--------------+---------------------------------------------------------------------------------------+ + """, config=True, ) diff --git a/binderhub/builder.py b/binderhub/builder.py index 2c7ff12e9..bbad59440 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -235,38 +235,18 @@ def _get_build_only_outcome(self): build_only_query_parameter = str( self.get_query_argument(name="build_only", default="") ) - # Describe how the traitlet and the query parameter need to be set together - # in order to get a build only behavior. - # Add this info table each time an unexpected (incorrect) config is observed. - log_message_eval_table = ( - "Table for evaluating whether or not the image will be launched after build" - "based on the values of the `require_build_only` traitlet and the `build_only` query parameter." - """ - | `require_build_only` trait | `build_only` query param | Outcome - ------------------------------------------------------------------------------------------------ - | false | missing | OK, image will be launched after build - | false | false | OK, image will be launched after build - | false | true | ERROR, building but not launching is not permitted when require_build_only == False - | true | missing | ERROR, query parameter must be explicitly set to true when require_build_only == True - | true | false | ERROR, query parameter must be explicitly set to true when require_build_only == True - | true | true | OK, image won't be launched after build - """ - ) - # Whether or not the image should only be built and not launched build_only_outcome = False if not require_build_only: if build_only_query_parameter.lower() == "true": raise HTTPError( log_message="Building but not launching is not permitted when " - "traitlet `require_build_only` is false and query parameter `build_only` is true! " - "See the table below for more info.\n\n" + log_message_eval_table + "traitlet `require_build_only` is false and query parameter `build_only` is true!" ) else: # Raise an error if the `build_only` query parameter is anything but `(T)true` if build_only_query_parameter.lower() != "true": raise HTTPError( - log_message="The `build_only=true` query parameter is required when traitlet `require_build_only` is false! " - "See the table below for more info.\n\n" + log_message_eval_table + log_message="The `build_only=true` query parameter is required when traitlet `require_build_only` is false!" ) # If we're here, it means a build only deployment is required build_only_outcome = True From 19d45bd1042ae8b726b265044c6e48e180282697 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 24 Apr 2023 13:07:33 +0000 Subject: [PATCH 29/48] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- binderhub/builder.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index bbad59440..edd745ef5 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -250,9 +250,8 @@ def _get_build_only_outcome(self): ) # If we're here, it means a build only deployment is required build_only_outcome = True - - return build_only_outcome + return build_only_outcome @authenticated async def get(self, provider_prefix, _unescaped_spec): From 96188edf8064dae493bc2bcee2cbeb40901bef64 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Mon, 24 Apr 2023 16:09:39 +0300 Subject: [PATCH 30/48] Add note about not supporting build only UI| --- binderhub/app.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/binderhub/app.py b/binderhub/app.py index e6df423cf..021de0057 100644 --- a/binderhub/app.py +++ b/binderhub/app.py @@ -809,6 +809,11 @@ def _template_path_default(self): +----------------------+--------------+---------------------------------------------------------------------------------------+ | true | true | OK, image won't be launched after build | +----------------------+--------------+---------------------------------------------------------------------------------------+ + + .. note:: + When this feature is enabled the only supported UX is the REST API based UX, as it's out of scope to support + a new UI based UX around not launching. + """, config=True, ) From f53636a449b175580c9c3d2a0e6f562057b2fa47 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Mon, 24 Apr 2023 16:12:32 +0300 Subject: [PATCH 31/48] Add fixtture to ensure no builds are tried when not available --- binderhub/tests/test_build.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index c30bcb7f1..301b62997 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -106,7 +106,7 @@ async def test_build(app, needs_build, needs_launch, always_build, slug, pytestc "app" ], # send param "require_build_only_app" to app fixture, so that it loads `require_build_only` configuration ) -async def test_build_only(app, build_only): +async def test_build_only(app, build_only, needs_build): """ Test build a repo that is very quick and easy to build. """ @@ -194,7 +194,7 @@ async def test_build_fail(app, needs_build, needs_launch, always_build): "app" ], # send param "require_build_only_app" to app fixture, so that it loads `require_build_only` configuration ) -async def test_build_only_fail(app, build_only, expected_error_msg): +async def test_build_only_fail(app, build_only, expected_error_msg, needs_build): """ Test the scenarios that are expected to fail when setting configs for building but no launching. From 702c57af492066026581ff52542d534aede215c6 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 28 Apr 2023 10:42:25 +0300 Subject: [PATCH 32/48] Render an error page when the build UI is accesed and require_build_only is True --- binderhub/main.py | 32 ++++++++++++++++++++++---------- binderhub/templates/error.html | 3 ++- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/binderhub/main.py b/binderhub/main.py index 6d302972e..4fccf6141 100644 --- a/binderhub/main.py +++ b/binderhub/main.py @@ -29,16 +29,28 @@ class MainHandler(BaseHandler): @authenticated def get(self): - self.render_template( - "index.html", - badge_base_url=self.get_badge_base_url(), - base_url=self.settings["base_url"], - submit=False, - google_analytics_code=self.settings["google_analytics_code"], - google_analytics_domain=self.settings["google_analytics_domain"], - extra_footer_scripts=self.settings["extra_footer_scripts"], - repo_providers=self.settings["repo_providers"], - ) + if self.settings["require_build_only"]: + self.render_template( + "error.html", + status_code=204, + status_message="UI deactivated", + message=""" + Because BinderHub.require_build_only was set, + only the REST API can be used. + An UI based around not launching is not supported. + """ + ) + else: + self.render_template( + "index.html", + badge_base_url=self.get_badge_base_url(), + base_url=self.settings["base_url"], + submit=False, + google_analytics_code=self.settings["google_analytics_code"], + google_analytics_domain=self.settings["google_analytics_domain"], + extra_footer_scripts=self.settings["extra_footer_scripts"], + repo_providers=self.settings["repo_providers"], + ) class ParameterizedMainHandler(BaseHandler): diff --git a/binderhub/templates/error.html b/binderhub/templates/error.html index 093d4e3e6..199331c80 100644 --- a/binderhub/templates/error.html +++ b/binderhub/templates/error.html @@ -10,7 +10,8 @@

- {{message}}. Note: Some errors disappear by refreshing the page. + {{message}}. + {{ "Note: Some errors disappear by refreshing the page" if status_code > 299 }}.

{% endblock main %} From a7d24183fe5210c3ffb50f5063bd46a38a169868 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 28 Apr 2023 07:59:10 +0000 Subject: [PATCH 33/48] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- binderhub/main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binderhub/main.py b/binderhub/main.py index 4fccf6141..bcd609307 100644 --- a/binderhub/main.py +++ b/binderhub/main.py @@ -38,7 +38,7 @@ def get(self): Because BinderHub.require_build_only was set, only the REST API can be used. An UI based around not launching is not supported. - """ + """, ) else: self.render_template( From 252987c440df94dd7d740617eb0ec5649b926ac5 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 28 Apr 2023 17:03:22 +0300 Subject: [PATCH 34/48] Just deactivate the / and /v2 endpoints when require_build_only is True --- binderhub/app.py | 13 +++++++++++-- binderhub/main.py | 33 ++++++++++----------------------- binderhub/templates/error.html | 3 +-- 3 files changed, 22 insertions(+), 27 deletions(-) diff --git a/binderhub/app.py b/binderhub/app.py index 021de0057..a5536f520 100644 --- a/binderhub/app.py +++ b/binderhub/app.py @@ -985,7 +985,17 @@ def initialize(self, *args, **kwargs): "Access-Control-Allow-Origin" ] = self.cors_allow_origin - handlers = [ + if self.require_build_only: + # Deactivate the UI's for the `/` and `/v2` endpoints + # as they won't make sense in a build only scenario + # when a REST API usage is expected + handlers = [] + else: + handlers = [ + (r"/v2/([^/]+)/(.+)", ParameterizedMainHandler), + (r"/", MainHandler), + ] + handlers += [ (r"/metrics", MetricsHandler), (r"/versions", VersionHandler), (r"/build/([^/]+)/(.+)", BuildHandler), @@ -1035,7 +1045,6 @@ def initialize(self, *args, **kwargs): (r"/about", AboutHandler), (r"/health", self.health_handler_class, {"hub_url": self.hub_url_local}), (r"/_config", ConfigHandler), - (r"/", MainHandler), (r".*", Custom404), ] handlers = self.add_url_prefix(self.base_url, handlers) diff --git a/binderhub/main.py b/binderhub/main.py index bcd609307..e17de20fd 100644 --- a/binderhub/main.py +++ b/binderhub/main.py @@ -29,29 +29,16 @@ class MainHandler(BaseHandler): @authenticated def get(self): - if self.settings["require_build_only"]: - self.render_template( - "error.html", - status_code=204, - status_message="UI deactivated", - message=""" - Because BinderHub.require_build_only was set, - only the REST API can be used. - An UI based around not launching is not supported. - """, - ) - else: - self.render_template( - "index.html", - badge_base_url=self.get_badge_base_url(), - base_url=self.settings["base_url"], - submit=False, - google_analytics_code=self.settings["google_analytics_code"], - google_analytics_domain=self.settings["google_analytics_domain"], - extra_footer_scripts=self.settings["extra_footer_scripts"], - repo_providers=self.settings["repo_providers"], - ) - + self.render_template( + "index.html", + badge_base_url=self.get_badge_base_url(), + base_url=self.settings["base_url"], + submit=False, + google_analytics_code=self.settings["google_analytics_code"], + google_analytics_domain=self.settings["google_analytics_domain"], + extra_footer_scripts=self.settings["extra_footer_scripts"], + repo_providers=self.settings["repo_providers"], + ) class ParameterizedMainHandler(BaseHandler): """Main handler that allows different parameter settings""" diff --git a/binderhub/templates/error.html b/binderhub/templates/error.html index 199331c80..093d4e3e6 100644 --- a/binderhub/templates/error.html +++ b/binderhub/templates/error.html @@ -10,8 +10,7 @@

- {{message}}. - {{ "Note: Some errors disappear by refreshing the page" if status_code > 299 }}. + {{message}}. Note: Some errors disappear by refreshing the page.

{% endblock main %} From 0203a2dd8fb730de0fa4c9d83da45745feb419a0 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 28 Apr 2023 14:03:56 +0000 Subject: [PATCH 35/48] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- binderhub/main.py | 1 + 1 file changed, 1 insertion(+) diff --git a/binderhub/main.py b/binderhub/main.py index e17de20fd..6d302972e 100644 --- a/binderhub/main.py +++ b/binderhub/main.py @@ -40,6 +40,7 @@ def get(self): repo_providers=self.settings["repo_providers"], ) + class ParameterizedMainHandler(BaseHandler): """Main handler that allows different parameter settings""" From 15dc9d333ed653b4497e8718f5671d39a04dd577 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 16 Jun 2023 13:43:24 +0300 Subject: [PATCH 36/48] Update build success metrics even if we don't launch after --- binderhub/builder.py | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index edd745ef5..b56d010f4 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -566,6 +566,10 @@ def _check_result(future): "message": message, "imageName": image_name, } + BUILD_TIME.labels(status="success").observe( + time.perf_counter() - build_starttime + ) + BUILD_COUNT.labels(status="success", **self.repo_metric_labels).inc() done = True elif progress.payload == ProgressEvent.BuildStatus.RUNNING: # start capturing build logs once the pod is running @@ -601,29 +605,25 @@ def _check_result(future): if build_only_outcome: return - else: + + if not failed: # Launch after building an image - if not failed: - BUILD_TIME.labels(status="success").observe( - time.perf_counter() - build_starttime + with LAUNCHES_INPROGRESS.track_inprogress(): + await self.launch(provider) + self.event_log.emit( + "binderhub.jupyter.org/launch", + 5, + { + "provider": provider.name, + "spec": spec, + "ref": ref, + "status": "success", + "build_token": self._have_build_token, + "origin": self.settings["normalized_origin"] + if self.settings["normalized_origin"] + else self.request.host, + }, ) - BUILD_COUNT.labels(status="success", **self.repo_metric_labels).inc() - with LAUNCHES_INPROGRESS.track_inprogress(): - await self.launch(provider) - self.event_log.emit( - "binderhub.jupyter.org/launch", - 5, - { - "provider": provider.name, - "spec": spec, - "ref": ref, - "status": "success", - "build_token": self._have_build_token, - "origin": self.settings["normalized_origin"] - if self.settings["normalized_origin"] - else self.request.host, - }, - ) # Don't close the eventstream immediately. # (javascript) eventstream clients reconnect automatically on dropped connections, From f3d174b2fd99ad3d75e932aff6d6d5a26496c95e Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 16 Jun 2023 10:44:28 +0000 Subject: [PATCH 37/48] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- binderhub/builder.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index b56d010f4..5433466ac 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -569,7 +569,9 @@ def _check_result(future): BUILD_TIME.labels(status="success").observe( time.perf_counter() - build_starttime ) - BUILD_COUNT.labels(status="success", **self.repo_metric_labels).inc() + BUILD_COUNT.labels( + status="success", **self.repo_metric_labels + ).inc() done = True elif progress.payload == ProgressEvent.BuildStatus.RUNNING: # start capturing build logs once the pod is running From c20ca5349db16269180537e5de2f5a3a26361d86 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Mon, 19 Jun 2023 10:30:01 +0300 Subject: [PATCH 38/48] Fix indentation --- binderhub/builder.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index 5433466ac..2244a9d05 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -612,20 +612,20 @@ def _check_result(future): # Launch after building an image with LAUNCHES_INPROGRESS.track_inprogress(): await self.launch(provider) - self.event_log.emit( - "binderhub.jupyter.org/launch", - 5, - { - "provider": provider.name, - "spec": spec, - "ref": ref, - "status": "success", - "build_token": self._have_build_token, - "origin": self.settings["normalized_origin"] - if self.settings["normalized_origin"] - else self.request.host, - }, - ) + self.event_log.emit( + "binderhub.jupyter.org/launch", + 5, + { + "provider": provider.name, + "spec": spec, + "ref": ref, + "status": "success", + "build_token": self._have_build_token, + "origin": self.settings["normalized_origin"] + if self.settings["normalized_origin"] + else self.request.host, + }, + ) # Don't close the eventstream immediately. # (javascript) eventstream clients reconnect automatically on dropped connections, From f7a4c3cef9c49b0ff2793a279b7fb023cb2b439b Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Thu, 21 Sep 2023 15:33:25 +0300 Subject: [PATCH 39/48] Replace require_build_only for a more clear enable_api_only_mode config --- binderhub/app.py | 129 ++++++++++++++++++++--------------------------- 1 file changed, 55 insertions(+), 74 deletions(-) diff --git a/binderhub/app.py b/binderhub/app.py index a5536f520..aade4e90d 100644 --- a/binderhub/app.py +++ b/binderhub/app.py @@ -786,34 +786,16 @@ def _template_path_default(self): help="Origin to use when emitting events. Defaults to hostname of request when empty", ) - require_build_only = Bool( + enable_api_only_mode = Bool( False, - help="""When enabled, the hub will no longer launch the image after the build when combined with the appropriate - `build_only` request query parameter. - - Below it's the table for evaluating whether or not the image will be launched after build, - based on the values of the `require_build_only` traitlet and the `build_only` query parameter. - - +----------------------+--------------+---------------------------------------------------------------------------------------+ - | `require_build_only` | `build_only` | Outcome | - +======================+==============+=======================================================================================+ - | false | missing | OK, image will be launched after build | - +----------------------+--------------+---------------------------------------------------------------------------------------+ - | false | false | OK, image will be launched after build | - +----------------------+--------------+---------------------------------------------------------------------------------------+ - | false | true | ERROR, building but not launching is not permitted when require_build_only == False | - +----------------------+--------------+---------------------------------------------------------------------------------------+ - | true | missing | ERROR, query parameter must be explicitly set to true when require_build_only == True | - +----------------------+--------------+---------------------------------------------------------------------------------------+ - | true | false | ERROR, query parameter must be explicitly set to true when require_build_only == True | - +----------------------+--------------+---------------------------------------------------------------------------------------+ - | true | true | OK, image won't be launched after build | - +----------------------+--------------+---------------------------------------------------------------------------------------+ - - .. note:: - When this feature is enabled the only supported UX is the REST API based UX, as it's out of scope to support - a new UI based UX around not launching. - + help=""" + When enabled, BinderHub will operate in an API only mode, + without a UI, and with the only registered endpoints being: + - /metrics + - /versions + - /build/([^/]+)/(.+) + - /health + - /_config """, config=True, ) @@ -975,7 +957,7 @@ def initialize(self, *args, **kwargs): "auth_enabled": self.auth_enabled, "event_log": self.event_log, "normalized_origin": self.normalized_origin, - "require_build_only": self.require_build_only, + "enable_api_only_mode": self.enable_api_only_mode, } ) if self.auth_enabled: @@ -985,7 +967,7 @@ def initialize(self, *args, **kwargs): "Access-Control-Allow-Origin" ] = self.cors_allow_origin - if self.require_build_only: + if self.enable_api_only_mode: # Deactivate the UI's for the `/` and `/v2` endpoints # as they won't make sense in a build only scenario # when a REST API usage is expected @@ -994,58 +976,57 @@ def initialize(self, *args, **kwargs): handlers = [ (r"/v2/([^/]+)/(.+)", ParameterizedMainHandler), (r"/", MainHandler), + (r"/repo/([^/]+)/([^/]+)(/.*)?", LegacyRedirectHandler), + # for backward-compatible mybinder.org badge URLs + # /assets/images/badge.svg + ( + r"/assets/(images/badge\.svg)", + tornado.web.StaticFileHandler, + {"path": self.tornado_settings["static_path"]}, + ), + # /badge.svg + ( + r"/(badge\.svg)", + tornado.web.StaticFileHandler, + {"path": os.path.join(self.tornado_settings["static_path"], "images")}, + ), + # /badge_logo.svg + ( + r"/(badge\_logo\.svg)", + tornado.web.StaticFileHandler, + {"path": os.path.join(self.tornado_settings["static_path"], "images")}, + ), + # /logo_social.png + ( + r"/(logo\_social\.png)", + tornado.web.StaticFileHandler, + {"path": os.path.join(self.tornado_settings["static_path"], "images")}, + ), + # /favicon_XXX.ico + ( + r"/(favicon\_fail\.ico)", + tornado.web.StaticFileHandler, + {"path": os.path.join(self.tornado_settings["static_path"], "images")}, + ), + ( + r"/(favicon\_success\.ico)", + tornado.web.StaticFileHandler, + {"path": os.path.join(self.tornado_settings["static_path"], "images")}, + ), + ( + r"/(favicon\_building\.ico)", + tornado.web.StaticFileHandler, + {"path": os.path.join(self.tornado_settings["static_path"], "images")}, + ), + (r"/about", AboutHandler), + (r".*", Custom404), ] handlers += [ (r"/metrics", MetricsHandler), (r"/versions", VersionHandler), (r"/build/([^/]+)/(.+)", BuildHandler), - (r"/v2/([^/]+)/(.+)", ParameterizedMainHandler), - (r"/repo/([^/]+)/([^/]+)(/.*)?", LegacyRedirectHandler), - # for backward-compatible mybinder.org badge URLs - # /assets/images/badge.svg - ( - r"/assets/(images/badge\.svg)", - tornado.web.StaticFileHandler, - {"path": self.tornado_settings["static_path"]}, - ), - # /badge.svg - ( - r"/(badge\.svg)", - tornado.web.StaticFileHandler, - {"path": os.path.join(self.tornado_settings["static_path"], "images")}, - ), - # /badge_logo.svg - ( - r"/(badge\_logo\.svg)", - tornado.web.StaticFileHandler, - {"path": os.path.join(self.tornado_settings["static_path"], "images")}, - ), - # /logo_social.png - ( - r"/(logo\_social\.png)", - tornado.web.StaticFileHandler, - {"path": os.path.join(self.tornado_settings["static_path"], "images")}, - ), - # /favicon_XXX.ico - ( - r"/(favicon\_fail\.ico)", - tornado.web.StaticFileHandler, - {"path": os.path.join(self.tornado_settings["static_path"], "images")}, - ), - ( - r"/(favicon\_success\.ico)", - tornado.web.StaticFileHandler, - {"path": os.path.join(self.tornado_settings["static_path"], "images")}, - ), - ( - r"/(favicon\_building\.ico)", - tornado.web.StaticFileHandler, - {"path": os.path.join(self.tornado_settings["static_path"], "images")}, - ), - (r"/about", AboutHandler), (r"/health", self.health_handler_class, {"hub_url": self.hub_url_local}), (r"/_config", ConfigHandler), - (r".*", Custom404), ] handlers = self.add_url_prefix(self.base_url, handlers) if self.extra_static_path: From 5cf641ddd285ba188df128432189ee949365174d Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Thu, 21 Sep 2023 16:00:29 +0300 Subject: [PATCH 40/48] Update how we decide if build only mode was activated --- binderhub/builder.py | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index 2244a9d05..58028465b 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -229,26 +229,19 @@ def set_default_headers(self): self.set_header("cache-control", "no-cache") def _get_build_only_outcome(self): - # Get the value of the `require_build_only` traitlet - require_build_only = self.settings.get("require_build_only", False) + # Get the value of the `enable_api_only_mode` traitlet + enable_api_only_mode = self.settings.get("enable_api_only_mode", False) # Get the value of the `build_only` query parameter if present build_only_query_parameter = str( self.get_query_argument(name="build_only", default="") ) build_only_outcome = False - if not require_build_only: - if build_only_query_parameter.lower() == "true": + if build_only_query_parameter.lower() == "true": + if not enable_api_only_mode: raise HTTPError( - log_message="Building but not launching is not permitted when " - "traitlet `require_build_only` is false and query parameter `build_only` is true!" + log_message="Building but not launching is not permitted when" + " the API only mode was not enabled by setting `enable_api_only_mode` to True. " ) - else: - # Raise an error if the `build_only` query parameter is anything but `(T)true` - if build_only_query_parameter.lower() != "true": - raise HTTPError( - log_message="The `build_only=true` query parameter is required when traitlet `require_build_only` is false!" - ) - # If we're here, it means a build only deployment is required build_only_outcome = True return build_only_outcome From 61d1a5d78773c93e853ecee80c345d101bff6b13 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Thu, 21 Sep 2023 17:13:50 +0300 Subject: [PATCH 41/48] Get the Custom404 handler in both cases --- binderhub/app.py | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/binderhub/app.py b/binderhub/app.py index aade4e90d..29e09efa2 100644 --- a/binderhub/app.py +++ b/binderhub/app.py @@ -967,13 +967,18 @@ def initialize(self, *args, **kwargs): "Access-Control-Allow-Origin" ] = self.cors_allow_origin - if self.enable_api_only_mode: - # Deactivate the UI's for the `/` and `/v2` endpoints - # as they won't make sense in a build only scenario - # when a REST API usage is expected - handlers = [] - else: - handlers = [ + handlers = [ + (r"/metrics", MetricsHandler), + (r"/versions", VersionHandler), + (r"/build/([^/]+)/(.+)", BuildHandler), + (r"/health", self.health_handler_class, {"hub_url": self.hub_url_local}), + (r"/_config", ConfigHandler), + ] + if not self.enable_api_only_mode: + # In API only mode the endpoints in the list below + # are unregistered as they don't make sense in a API only scenario + handlers += [ + (r"/about", AboutHandler), (r"/v2/([^/]+)/(.+)", ParameterizedMainHandler), (r"/", MainHandler), (r"/repo/([^/]+)/([^/]+)(/.*)?", LegacyRedirectHandler), @@ -1018,16 +1023,9 @@ def initialize(self, *args, **kwargs): tornado.web.StaticFileHandler, {"path": os.path.join(self.tornado_settings["static_path"], "images")}, ), - (r"/about", AboutHandler), - (r".*", Custom404), ] - handlers += [ - (r"/metrics", MetricsHandler), - (r"/versions", VersionHandler), - (r"/build/([^/]+)/(.+)", BuildHandler), - (r"/health", self.health_handler_class, {"hub_url": self.hub_url_local}), - (r"/_config", ConfigHandler), - ] + # This needs to be the last handler in the list, because it needs to match "everything else" + handlers.append((r".*", Custom404)) handlers = self.add_url_prefix(self.base_url, handlers) if self.extra_static_path: handlers.insert( From 08d4f1d9a69d78f7d6e559178bbcbc9ce8e7244f Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Thu, 21 Sep 2023 17:14:09 +0300 Subject: [PATCH 42/48] update message --- binderhub/builder.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index 58028465b..742110c02 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -432,8 +432,8 @@ async def get(self, provider_prefix, _unescaped_spec): { "phase": "info", "imageName": image_name, - "message": "Both `require_build_only` traitlet, and the query parameter `build_only` are true, " - "so the built image will not be launched\n", + "message": "Because the API only mode was enabled and the query parameter `build_only` was set to true, " + "the built image will not be launched\n", } ) if image_found: From 368cad76179f8217b1cf55cc6494d8bfcb14ec16 Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Thu, 21 Sep 2023 17:14:34 +0300 Subject: [PATCH 43/48] Update the test config --- testing/local-binder-mocked-hub/binderhub_config.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/testing/local-binder-mocked-hub/binderhub_config.py b/testing/local-binder-mocked-hub/binderhub_config.py index 58ab8b6d3..7136d43a1 100644 --- a/testing/local-binder-mocked-hub/binderhub_config.py +++ b/testing/local-binder-mocked-hub/binderhub_config.py @@ -16,8 +16,12 @@ c.BinderHub.builder_required = False c.BinderHub.repo_providers = {"gh": FakeProvider} c.BinderHub.build_class = FakeBuild -# Uncomment the following line to enable BinderHub to not launch the image after build -# c.BinderHub.require_build_only = True + +# Uncomment the following line to enable BinderHub's API only mode +# With this, we can then use the `build_only` query parameter in the request +# to not launch the image after build + +c.BinderHub.enable_api_only_mode = True c.BinderHub.about_message = "Hello world." c.BinderHub.banner_message = 'This is headline news.' From 693baf9f51cd1f38bb98abe48e1dfe30e750fa1b Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Thu, 21 Sep 2023 17:45:18 +0300 Subject: [PATCH 44/48] Update tests --- binderhub/tests/conftest.py | 14 ++++++------- binderhub/tests/test_build.py | 38 +++++++++++++---------------------- 2 files changed, 21 insertions(+), 31 deletions(-) diff --git a/binderhub/tests/conftest.py b/binderhub/tests/conftest.py index c9361faed..2252246ab 100644 --- a/binderhub/tests/conftest.py +++ b/binderhub/tests/conftest.py @@ -253,23 +253,23 @@ def app(request, io_loop, _binderhub_config): app._configured_bhub = BinderHub(config=_binderhub_config) return app - build_only = False + api_only_app = False if hasattr(request, "param"): if request.param == "app_with_auth_config": # load conf for auth test cfg = PyFileConfigLoader(binderhub_config_auth_additions_path).load_config() _binderhub_config.merge(cfg) - elif request.param == "app_with_require_build_only": - # load conf that sets BinderHub.require_build_only = True - cfg = Config({"BinderHub": {"require_build_only": True}}) + elif request.param == "api_only_app": + # load conf that sets BinderHub.enable_api_only_mode = True + cfg = Config({"BinderHub": {"enable_api_only_mode": True}}) _binderhub_config.merge(cfg) - build_only = True + api_only_app = True - if not build_only: + if not api_only_app: # load conf that sets BinderHub.require_build_only = False # otherwise because _binderhub_config has a session scope, # any previous set of require_build_only to True will stick around - cfg = Config({"BinderHub": {"require_build_only": False}}) + cfg = Config({"BinderHub": {"enable_api_only_mode": False}}) _binderhub_config.merge(cfg) bhub = BinderHub.instance(config=_binderhub_config) diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index 301b62997..49e2b3cc7 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -98,22 +98,22 @@ async def test_build(app, needs_build, needs_launch, always_build, slug, pytestc @pytest.mark.asyncio(timeout=900) @pytest.mark.parametrize( - "app,build_only", + "app,build_only_query_param", [ - ("app_with_require_build_only", "True"), + ("api_only_app", "True"), ], indirect=[ "app" - ], # send param "require_build_only_app" to app fixture, so that it loads `require_build_only` configuration + ], # send param "api_only_app" to app fixture, so that it loads `enable_api_only_mode` configuration ) -async def test_build_only(app, build_only, needs_build): +async def test_build_only(app, build_only_query_param, needs_build): """ Test build a repo that is very quick and easy to build. """ slug = "gh/binderhub-ci-repos/cached-minimal-dockerfile/HEAD" build_url = f"{app.url}/build/{slug}" r = await async_requests.get( - build_url, stream=True, params={"build_only": build_only} + build_url, stream=True, params={"build_only": build_only_query_param} ) r.raise_for_status() events = [] @@ -130,7 +130,7 @@ async def test_build_only(app, build_only, needs_build): break if event.get("phase") == "info": assert ( - "Both `require_build_only` traitlet, and the query parameter `build_only` are true" + "Both `enable_api_only_node` traitlet, and the query parameter `build_only` are true" in event["message"] ) if event.get("phase") == "launching" and not event["message"].startswith( @@ -172,18 +172,8 @@ async def test_build_fail(app, needs_build, needs_launch, always_build): @pytest.mark.asyncio(timeout=120) @pytest.mark.parametrize( - "app,build_only,expected_error_msg", + "app,build_only_query_param,expected_error_msg", [ - ( - "app_with_require_build_only", - False, - "The `build_only=true` query parameter is required", - ), - ( - "app_with_require_build_only", - "", - "The `build_only=true` query parameter is required", - ), ( "app_without_require_build_only", True, @@ -194,27 +184,27 @@ async def test_build_fail(app, needs_build, needs_launch, always_build): "app" ], # send param "require_build_only_app" to app fixture, so that it loads `require_build_only` configuration ) -async def test_build_only_fail(app, build_only, expected_error_msg, needs_build): +async def test_build_only_fail(app, build_only_query_param, expected_error_msg, needs_build): """ Test the scenarios that are expected to fail when setting configs for building but no launching. Table for evaluating whether or not the image will be launched after build based on the values of - the `require_build_only` traitlet and the `build_only` query parameter. + the `enable_api_only_mode` traitlet and the `build_only` query parameter. - | `require_build_only` trait | `build_only` query param | Outcome + | `enable_api_only_mode` trait | `build_only` query param | Outcome ------------------------------------------------------------------------------------------------ | false | missing | OK, image will be launched after build | false | false | OK, image will be launched after build - | false | true | ERROR, building but not launching is not permitted when require_build_only == False - | true | missing | ERROR, query parameter must be explicitly set to true when require_build_only == True - | true | false | ERROR, query parameter must be explicitly set to true when require_build_only == True + | false | true | ERROR, building but not launching is not permitted when UI is still enabled + | true | missing | OK, image will be launched after build + | true | false | OK, image will be launched after build | true | true | OK, image won't be launched after build """ slug = "gh/binderhub-ci-repos/cached-minimal-dockerfile/HEAD" build_url = f"{app.url}/build/{slug}" r = await async_requests.get( - build_url, stream=True, params={"build_only": build_only} + build_url, stream=True, params={"build_only": build_only_query_param} ) r.raise_for_status() failed_events = 0 From b2925dcd520e0b306f6d53df8a95773c2a4500ff Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 21 Sep 2023 14:45:51 +0000 Subject: [PATCH 45/48] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- binderhub/app.py | 36 +++++++++++++++++++++++++++++------ binderhub/tests/test_build.py | 4 +++- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/binderhub/app.py b/binderhub/app.py index 29e09efa2..2d925104e 100644 --- a/binderhub/app.py +++ b/binderhub/app.py @@ -993,35 +993,59 @@ def initialize(self, *args, **kwargs): ( r"/(badge\.svg)", tornado.web.StaticFileHandler, - {"path": os.path.join(self.tornado_settings["static_path"], "images")}, + { + "path": os.path.join( + self.tornado_settings["static_path"], "images" + ) + }, ), # /badge_logo.svg ( r"/(badge\_logo\.svg)", tornado.web.StaticFileHandler, - {"path": os.path.join(self.tornado_settings["static_path"], "images")}, + { + "path": os.path.join( + self.tornado_settings["static_path"], "images" + ) + }, ), # /logo_social.png ( r"/(logo\_social\.png)", tornado.web.StaticFileHandler, - {"path": os.path.join(self.tornado_settings["static_path"], "images")}, + { + "path": os.path.join( + self.tornado_settings["static_path"], "images" + ) + }, ), # /favicon_XXX.ico ( r"/(favicon\_fail\.ico)", tornado.web.StaticFileHandler, - {"path": os.path.join(self.tornado_settings["static_path"], "images")}, + { + "path": os.path.join( + self.tornado_settings["static_path"], "images" + ) + }, ), ( r"/(favicon\_success\.ico)", tornado.web.StaticFileHandler, - {"path": os.path.join(self.tornado_settings["static_path"], "images")}, + { + "path": os.path.join( + self.tornado_settings["static_path"], "images" + ) + }, ), ( r"/(favicon\_building\.ico)", tornado.web.StaticFileHandler, - {"path": os.path.join(self.tornado_settings["static_path"], "images")}, + { + "path": os.path.join( + self.tornado_settings["static_path"], "images" + ) + }, ), ] # This needs to be the last handler in the list, because it needs to match "everything else" diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index 49e2b3cc7..bbdaead04 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -184,7 +184,9 @@ async def test_build_fail(app, needs_build, needs_launch, always_build): "app" ], # send param "require_build_only_app" to app fixture, so that it loads `require_build_only` configuration ) -async def test_build_only_fail(app, build_only_query_param, expected_error_msg, needs_build): +async def test_build_only_fail( + app, build_only_query_param, expected_error_msg, needs_build +): """ Test the scenarios that are expected to fail when setting configs for building but no launching. From 6252eb6aea418001139b9d585fe3e4337573b61e Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Fri, 22 Sep 2023 11:26:46 +0300 Subject: [PATCH 46/48] Updat log message and assert --- binderhub/builder.py | 4 ++-- binderhub/tests/test_build.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index 742110c02..583a2871d 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -432,8 +432,8 @@ async def get(self, provider_prefix, _unescaped_spec): { "phase": "info", "imageName": image_name, - "message": "Because the API only mode was enabled and the query parameter `build_only` was set to true, " - "the built image will not be launched\n", + "message": "The built image will not be launched " + "because the API only mode was enabled and the query parameter `build_only` was set to true\n", } ) if image_found: diff --git a/binderhub/tests/test_build.py b/binderhub/tests/test_build.py index bbdaead04..21adc22e4 100644 --- a/binderhub/tests/test_build.py +++ b/binderhub/tests/test_build.py @@ -130,7 +130,7 @@ async def test_build_only(app, build_only_query_param, needs_build): break if event.get("phase") == "info": assert ( - "Both `enable_api_only_node` traitlet, and the query parameter `build_only` are true" + "The built image will not be launched because the API only mode was enabled and the query parameter `build_only` was set to true" in event["message"] ) if event.get("phase") == "launching" and not event["message"].startswith( From 114b1950b065c7713f2fc32b1384de3b9632dade Mon Sep 17 00:00:00 2001 From: Georgiana Dolocan Date: Tue, 10 Oct 2023 11:30:12 +0300 Subject: [PATCH 47/48] Drop the outcome suffix for clariry and specify a status code for the http error Co-authored-by: Min RK --- binderhub/app.py | 1 + binderhub/builder.py | 19 ++++++++++--------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/binderhub/app.py b/binderhub/app.py index 2d925104e..2d6f5ca54 100644 --- a/binderhub/app.py +++ b/binderhub/app.py @@ -796,6 +796,7 @@ def _template_path_default(self): - /build/([^/]+)/(.+) - /health - /_config + - /* -> shows a 404 page """, config=True, ) diff --git a/binderhub/builder.py b/binderhub/builder.py index 583a2871d..c41a11ff0 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -228,23 +228,24 @@ def set_default_headers(self): self.set_header("content-type", "text/event-stream") self.set_header("cache-control", "no-cache") - def _get_build_only_outcome(self): + def _get_build_only(self): # Get the value of the `enable_api_only_mode` traitlet enable_api_only_mode = self.settings.get("enable_api_only_mode", False) # Get the value of the `build_only` query parameter if present build_only_query_parameter = str( self.get_query_argument(name="build_only", default="") ) - build_only_outcome = False + build_only = False if build_only_query_parameter.lower() == "true": if not enable_api_only_mode: raise HTTPError( + status_code=400, log_message="Building but not launching is not permitted when" " the API only mode was not enabled by setting `enable_api_only_mode` to True. " ) - build_only_outcome = True + build_only = True - return build_only_outcome + return build_only @authenticated async def get(self, provider_prefix, _unescaped_spec): @@ -426,8 +427,8 @@ async def get(self, provider_prefix, _unescaped_spec): else: image_found = True - build_only_outcome = self._get_build_only_outcome() - if build_only_outcome: + build_only = self._get_build_only() + if build_only: await self.emit( { "phase": "info", @@ -437,7 +438,7 @@ async def get(self, provider_prefix, _unescaped_spec): } ) if image_found: - if build_only_outcome: + if build_only: await self.emit( { "phase": "ready", @@ -549,7 +550,7 @@ def _check_result(future): # nothing to do, just waiting continue elif progress.payload == ProgressEvent.BuildStatus.BUILT: - if build_only_outcome: + if build_only: message = "Done! Image built\n" phase = "ready" else: @@ -598,7 +599,7 @@ def _check_result(future): ).inc() await self.emit(event) - if build_only_outcome: + if build_only: return if not failed: From d5f686d84d059e83ad75718dc092df9c497bb6c7 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 10 Oct 2023 08:31:38 +0000 Subject: [PATCH 48/48] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- binderhub/builder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/binderhub/builder.py b/binderhub/builder.py index c41a11ff0..9cc41550f 100644 --- a/binderhub/builder.py +++ b/binderhub/builder.py @@ -241,7 +241,7 @@ def _get_build_only(self): raise HTTPError( status_code=400, log_message="Building but not launching is not permitted when" - " the API only mode was not enabled by setting `enable_api_only_mode` to True. " + " the API only mode was not enabled by setting `enable_api_only_mode` to True. ", ) build_only = True