From 377e236366c5eb4333b6d6366c410d1c0d6c5ce7 Mon Sep 17 00:00:00 2001 From: Michael Woolnough Date: Wed, 6 Nov 2024 11:46:51 +0000 Subject: [PATCH 1/3] Determine reason for build failure and store env metadata. --- schema.graphql | 1 + softpack_core/artifacts.py | 1 + softpack_core/schemas/environment.py | 25 +++++-- softpack_core/service.py | 95 +++++++++++++++++---------- tests/integration/test_environment.py | 67 +++++++++++++++++++ 5 files changed, 150 insertions(+), 39 deletions(-) diff --git a/schema.graphql b/schema.graphql index 16a8a5d..befc8f3 100644 --- a/schema.graphql +++ b/schema.graphql @@ -39,6 +39,7 @@ type Environment { state: State tags: [String!]! username: String + failureReason: String hidden: Boolean! created: Int! cachedEnvs: [Environment!]! diff --git a/softpack_core/artifacts.py b/softpack_core/artifacts.py index b9380c4..c8be6d5 100644 --- a/softpack_core/artifacts.py +++ b/softpack_core/artifacts.py @@ -179,6 +179,7 @@ def spec(self) -> Box: info["force_hidden"] = getattr(metadata, "force_hidden", False) info["created"] = getattr(metadata, "created", 0) info["username"] = getattr(metadata, "username", None) + info["failure_reason"] = getattr(metadata, "failure_reason", None) info["interpreters"] = Interpreters() diff --git a/softpack_core/schemas/environment.py b/softpack_core/schemas/environment.py index 320a71c..d652810 100644 --- a/softpack_core/schemas/environment.py +++ b/softpack_core/schemas/environment.py @@ -335,6 +335,7 @@ class Environment: state: Optional[State] tags: list[str] username: Optional[str] + failure_reason: Optional[str] hidden: bool created: int cachedEnvs: list["Environment"] = field(default_factory=list) @@ -437,6 +438,7 @@ def from_artifact(cls, obj: Artifacts.Object) -> Optional["Environment"]: type=spec.get("type", ""), tags=spec.tags, username=spec.username, + failure_reason=spec.failure_reason, hidden=spec.hidden, created=spec.created, interpreters=spec.get("interpreters", Interpreters()), @@ -736,14 +738,21 @@ def set_hidden( return HiddenSuccess(message="Hidden metadata set") - def remove_username(cls) -> None: - """Remove the username metadata from the meta.yaml file.""" + def update_metadata(cls, key: str, value: str | None) -> None: + """Takes a key and sets the value unless value is None.""" metadata = cls.read_metadata(cls.path, cls.name) - del metadata["username"] + if value is None: + del metadata[key] + else: + metadata[key] = value cls.store_metadata(Path(cls.path, cls.name), metadata) + def remove_username(cls) -> None: + """Remove the username metadata from the meta.yaml file.""" + cls.update_metadata("username", None) + @classmethod def delete(cls, name: str, path: str) -> DeleteResponse: # type: ignore """Delete an Environment. @@ -903,7 +912,9 @@ async def write_artifact( @classmethod async def write_artifacts( - cls, folder_path: str, files: list[Union[Upload, UploadFile]] + cls, + folder_path: str, + files: list[Union[Upload, UploadFile, Tuple[str, str]]], ) -> WriteArtifactResponse: # type: ignore """Add one or more files to the Artifacts repo. @@ -914,7 +925,11 @@ async def write_artifacts( try: new_files: List[Tuple[str, Union[str, UploadFile]]] = [] for file in files: - if isinstance(file, starlette.datastructures.UploadFile): + if isinstance(file, tuple): + new_files.append( + cast(Tuple[str, Union[str, UploadFile]], file) + ) + elif isinstance(file, starlette.datastructures.UploadFile): new_files.append( (file.filename or "", cast(UploadFile, file)) ) diff --git a/softpack_core/service.py b/softpack_core/service.py index c506b86..ca7f746 100644 --- a/softpack_core/service.py +++ b/softpack_core/service.py @@ -9,11 +9,13 @@ import urllib.parse from email.mime.text import MIMEText from pathlib import Path +from typing import Tuple, Union, cast import typer import uvicorn import yaml from fastapi import APIRouter, Request, Response, UploadFile +from strawberry.file_uploads import Upload from typer import Typer from typing_extensions import Annotated @@ -111,48 +113,73 @@ async def upload_artifacts( # type: ignore[no-untyped-def] path = Path(env_path) env = Environment.get_env(path.parent, path.name) newState = State.queued + files = cast(list[Union[Upload, UploadFile, Tuple[str, str]]], file) - for f in file: - if f.filename == artifacts.builder_out: - newState = State.failed + if env: + for i in range(len(file)): + f = file[i] + if f.filename == artifacts.builder_out: + newState = State.failed - break + contents = cast(str, (await f.read()).decode()) - if f.filename == artifacts.module_file: - newState = State.ready + if ( + "concretization failed for the following reasons:" + in contents + ): + env.update_metadata("failure_reason", "concretization") + else: + env.update_metadata("failure_reason", "build") - break + files[i] = (f.filename, contents) - if ( - newState != State.queued - and env is not None - and env.username is not None - and env.username != "" - ): - envEmailConfig = app.settings.environments - m = ( - "built sucessfully" - if newState == State.ready - else "failed to build" - ) - message = ( - f"Hi {env.username},\n" - + "\n" - + f"Your environment, {env_path}, has {m}.\n" - + "\n" - + "SoftPack Team" - ) + break - subject = ( - "Your environment is ready!" - if newState == State.ready - else "Your environment failed to build" - ) + if f.filename == artifacts.module_file: + newState = State.ready + + break + + if ( + newState != State.queued + and env.username is not None + and env.username != "" + ): + envEmailConfig = app.settings.environments + m = ( + "built sucessfully" + if newState == State.ready + else "failed to build" + ) + + e = ( + "" + if newState == State.ready + else + "\nThe error was a version conflict. Try relaxing which versions you've specified.\n" + if env.failure_reason == "concretization" + else "\nThe error was a build error. Contact your softpack administrator.\n" + ) + + message = ( + f"Hi {env.username},\n" + + "\n" + + f"Your environment, {env_path}, has {m}.\n" + + e + + "\n" + + "SoftPack Team" + ) + + subject = ( + "Your environment is ready!" + if newState == State.ready + else "Your environment failed to build" + ) - send_email(envEmailConfig, message, subject, env.username) - env.remove_username() + send_email(envEmailConfig, message, subject, env.username) + env.remove_username() - resp = await Environment.write_artifacts(env_path, file) + resp = await Environment.write_artifacts(env_path, files) if not isinstance(resp, WriteArtifactSuccess): raise Exception(resp) diff --git a/tests/integration/test_environment.py b/tests/integration/test_environment.py index 7b65187..4b68ebb 100644 --- a/tests/integration/test_environment.py +++ b/tests/integration/test_environment.py @@ -429,6 +429,7 @@ async def test_email_on_build_complete( assert send_email.call_args[0][0] == app.settings.environments assert "failed to build" in send_email.call_args[0][1] + assert "build error" in send_email.call_args[0][1] assert send_email.call_args[0][2] == "Your environment failed to build" assert send_email.call_args[0][3] == "me" @@ -452,6 +453,72 @@ async def test_email_on_build_complete( assert send_email.call_count == 2 + result = Environment.create(testable_env_input) + assert isinstance(result, CreateEnvironmentSuccess) + + client = TestClient(app.router) + resp = client.post( + url="/upload?" + + testable_env_input.path + + "/" + + testable_env_input.name, + files=[ + ("file", (Artifacts.builder_out, "concretization failed for the following reasons:")), + ], + ) + assert resp.status_code == 200 + assert resp.json().get("message") == "Successfully written artifact(s)" + + assert send_email.call_count == 2 + + assert send_email.call_args[0][0] == app.settings.environments + assert "failed to build" in send_email.call_args[0][1] + assert "version conflict" in send_email.call_args[0][1] + assert send_email.call_args[0][2] == "Your environment failed to build" + assert send_email.call_args[0][3] == "me" + + +def test_failure_reason_from_build_log( + httpx_post, send_email, testable_env_input +): + result = Environment.create(testable_env_input) + assert isinstance(result, CreateEnvironmentSuccess) + + client = TestClient(app.router) + client.post( + url="/upload?" + + testable_env_input.path + + "/" + + testable_env_input.name, + files=[ + ("file", (Artifacts.builder_out, "")), + ], + ) + + env = Environment.get_env(testable_env_input.path, testable_env_input.name) + + assert env.failure_reason == "build" + + client.post( + url="/upload?" + + testable_env_input.path + + "/" + + testable_env_input.name, + files=[ + ( + "file", + ( + Artifacts.builder_out, + "concretization failed for the following reasons:", + ), + ), + ], + ) + + env = Environment.get_env(testable_env_input.path, testable_env_input.name) + + assert env.failure_reason == "concretization" + def test_iter(testable_env_input, mocker): get_mock = mocker.patch("httpx.get") From 6e5c26a96d73ebee4f2f70fbaab3e9e32219ab1a Mon Sep 17 00:00:00 2001 From: Michael Woolnough Date: Wed, 6 Nov 2024 11:47:28 +0000 Subject: [PATCH 2/3] Include build failure reason in email sent to users. --- softpack_core/service.py | 11 +++++++---- tests/integration/test_environment.py | 17 ++++++++++++++--- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/softpack_core/service.py b/softpack_core/service.py index ca7f746..3f77ecd 100644 --- a/softpack_core/service.py +++ b/softpack_core/service.py @@ -128,8 +128,10 @@ async def upload_artifacts( # type: ignore[no-untyped-def] in contents ): env.update_metadata("failure_reason", "concretization") + env.failure_reason = "concretization" else: env.update_metadata("failure_reason", "build") + env.failure_reason = "build" files[i] = (f.filename, contents) @@ -155,10 +157,11 @@ async def upload_artifacts( # type: ignore[no-untyped-def] e = ( "" if newState == State.ready - else - "\nThe error was a version conflict. Try relaxing which versions you've specified.\n" - if env.failure_reason == "concretization" - else "\nThe error was a build error. Contact your softpack administrator.\n" + else "\nThe error was a build error. " + + "Contact your softpack administrator.\n" + if env.failure_reason == "build" + else "\nThe error was a version conflict. " + + "Try relaxing which versions you've specified.\n" ) message = ( diff --git a/tests/integration/test_environment.py b/tests/integration/test_environment.py index 4b68ebb..a32da7e 100644 --- a/tests/integration/test_environment.py +++ b/tests/integration/test_environment.py @@ -393,6 +393,7 @@ async def test_email_on_build_complete( assert send_email.call_args[0][0] == app.settings.environments assert "built sucessfully" in send_email.call_args[0][1] + assert "The error was" not in send_email.call_args[0][1] assert send_email.call_args[0][2] == "Your environment is ready!" assert send_email.call_args[0][3] == "me" @@ -429,7 +430,10 @@ async def test_email_on_build_complete( assert send_email.call_args[0][0] == app.settings.environments assert "failed to build" in send_email.call_args[0][1] - assert "build error" in send_email.call_args[0][1] + assert ( + "The error was a build error. Contact your softpack administrator." + in send_email.call_args[0][1] + ) assert send_email.call_args[0][2] == "Your environment failed to build" assert send_email.call_args[0][3] == "me" @@ -453,6 +457,7 @@ async def test_email_on_build_complete( assert send_email.call_count == 2 + testable_env_input.username = "me" result = Environment.create(testable_env_input) assert isinstance(result, CreateEnvironmentSuccess) @@ -463,13 +468,19 @@ async def test_email_on_build_complete( + "/" + testable_env_input.name, files=[ - ("file", (Artifacts.builder_out, "concretization failed for the following reasons:")), + ( + "file", + ( + Artifacts.builder_out, + "concretization failed for the following reasons:", + ), + ), ], ) assert resp.status_code == 200 assert resp.json().get("message") == "Successfully written artifact(s)" - assert send_email.call_count == 2 + assert send_email.call_count == 3 assert send_email.call_args[0][0] == app.settings.environments assert "failed to build" in send_email.call_args[0][1] From 72f39df891e92b00f4a0c5148338b0ca46b355ae Mon Sep 17 00:00:00 2001 From: Michael Woolnough Date: Wed, 6 Nov 2024 12:20:17 +0000 Subject: [PATCH 3/3] Add option to send copy of build failure email (and recipe request email) to a second (admin) address. --- README.md | 2 ++ softpack_core/config/models.py | 1 + softpack_core/service.py | 18 +++++++++++++++--- tests/unit/test_service.py | 10 +++++++++- 4 files changed, 27 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 679528e..cf58fde 100644 --- a/README.md +++ b/README.md @@ -222,12 +222,14 @@ builder: recipes: # Used to send recipe request emails; needs toAddr, fromAddr, and smtp to be set to send out emails. toAddr: Optional[str] # Address to which recipe requests will be sent, uses of '{}' will be replaced with the username of the requestor. fromAddr: Optional[str] # Address from which recipe requests will be sent, uses of '{}' will be replaced by the same username as above. + adminAddr: # Address to send a copy of the recipe request email to. smtp: Optional[str] # Address to an SMTP relay localHostname: Optional[str] # Hostname to use for SMTP HELO. environments: # Used to send environment update emails; needs toAddr, fromAddr, and smtp to be set to send out emails. toAddr: Optional[str] # Address to which environment build failure/success emails are sent, uses of '{}' will be replaced with the username associated with the environment. fromAddr: Optional[str] # Address from which environment updates will be sent, uses of '{}' will be replaced by the same username as above. + adminAddr: Optional[str] # Address to send a copy of failure emails to. smtp: Optional[str] # Address to an SMTP relay localHostname: Optional[str] # Hostname to use for SMTP HELO. ``` diff --git a/softpack_core/config/models.py b/softpack_core/config/models.py index de44c15..9fc9e5c 100644 --- a/softpack_core/config/models.py +++ b/softpack_core/config/models.py @@ -85,5 +85,6 @@ class EmailConfig(BaseModel): toAddr: Optional[str] fromAddr: Optional[str] + adminAddr: Optional[str] smtp: Optional[str] localHostname: Optional[str] diff --git a/softpack_core/service.py b/softpack_core/service.py index 3f77ecd..be56dca 100644 --- a/softpack_core/service.py +++ b/softpack_core/service.py @@ -179,7 +179,13 @@ async def upload_artifacts( # type: ignore[no-untyped-def] else "Your environment failed to build" ) - send_email(envEmailConfig, message, subject, env.username) + send_email( + envEmailConfig, + message, + subject, + env.username, + newState != State.ready, + ) env.remove_username() resp = await Environment.write_artifacts(env_path, files) @@ -407,7 +413,11 @@ async def recipe_description( # type: ignore[no-untyped-def] def send_email( - emailConfig: EmailConfig, message: str, subject: str, username: str + emailConfig: EmailConfig, + message: str, + subject: str, + username: str, + sendAdmin: bool = True, ) -> None: """The send_email functions sends an email.""" if ( @@ -434,7 +444,9 @@ def send_email( s = smtplib.SMTP(emailConfig.smtp, local_hostname=localhostname) s.sendmail( fromAddr, - [toAddr], + [toAddr, emailConfig.adminAddr] + if sendAdmin and emailConfig.adminAddr is not None + else [toAddr], msg.as_string(), ) s.quit() diff --git a/tests/unit/test_service.py b/tests/unit/test_service.py index 45aac17..edb3e3d 100644 --- a/tests/unit/test_service.py +++ b/tests/unit/test_service.py @@ -60,6 +60,7 @@ def test_send_email(mocker): emailConfig = EmailConfig( fromAddr="{}@domain.com", toAddr="{}@other-domain.com", + adminAddr="admin@domain.com", smtp="host.mail.com", localHostname="something", ) @@ -72,6 +73,13 @@ def test_send_email(mocker): mock_SMTP.return_value.sendmail.call_args[0][0] == "USERNAME2@domain.com" ) + assert mock_SMTP.return_value.sendmail.call_args[0][1] == [ + "USERNAME2@other-domain.com", + "admin@domain.com", + ] + + send_email(emailConfig, "MESSAGE2", "SUBJECT2", "USERNAME2", False) + assert mock_SMTP.return_value.sendmail.call_count == 3 assert mock_SMTP.return_value.sendmail.call_args[0][1] == [ "USERNAME2@other-domain.com" ] @@ -82,4 +90,4 @@ def test_send_email(mocker): emailConfig = EmailConfig() send_email(emailConfig, "MESSAGE3", "SUBJECT3", "USERNAME3") - assert mock_SMTP.return_value.sendmail.call_count == 2 + assert mock_SMTP.return_value.sendmail.call_count == 3