Repository: omnivector-solutions/jobbergate (language: python)

Issue:
Armada-460 move job script files into s3
#### What
Drop the column `job_script_data_as_string` from `job_scripts` table. They should be put in S3 just like the application files.

#### Why
We are currently storing the files in our database. There have been issues with this around moving the files back and forth between the CLI and the API.

`Task`: https://app.clickup.com/t/18022949/ARMADA-460

---

#### Peer Review
Please follow the upstream omnivector documentation concerning
[peer-review guidelines](https://github.com/omnivector-solutions/Documentation/blob/main/Contributing/pr_review_standards.md#peer-review).



Discussion / hints:
Question: will the job scripts be in the same bucket as the applications?
> Question: will the job scripts be in the same bucket as the applications?

They are for now. But we can change it if necessary. Do you think separated buckets are better?
Task linked: [ARMADA-460 Move job_script files into S3](https://app.clickup.com/t/18022949/ARMADA-460)

Fix patch:
diff --git a/jobbergate-api/CHANGELOG.rst b/jobbergate-api/CHANGELOG.rst
index 5c127768..cfc0c491 100644
--- a/jobbergate-api/CHANGELOG.rst
+++ b/jobbergate-api/CHANGELOG.rst
@@ -6,6 +6,7 @@ This file keeps track of all notable changes to jobbergate-api
 
 Unreleased
 ----------
+- Fix API was not sending job-script files to the agent.
 
 3.2.3 -- 2022-08-01
 -------------------
diff --git a/jobbergate-api/jobbergate_api/apps/job_submissions/routers.py b/jobbergate-api/jobbergate_api/apps/job_submissions/routers.py
index b1b88432..24e51e73 100644
--- a/jobbergate-api/jobbergate_api/apps/job_submissions/routers.py
+++ b/jobbergate-api/jobbergate_api/apps/job_submissions/routers.py
@@ -26,6 +26,7 @@
 from jobbergate_api.apps.permissions import Permissions
 from jobbergate_api.email_notification import notify_submission_rejected
 from jobbergate_api.pagination import Pagination, ok_response, package_response
+from jobbergate_api.s3_manager import s3man_jobscripts
 from jobbergate_api.security import IdentityClaims, guard
 from jobbergate_api.storage import (
     INTEGRITY_CHECK_EXCEPTIONS,
@@ -299,6 +300,7 @@ async def job_submissions_agent_pending(
                 job_submissions_table.c.id,
                 job_submissions_table.c.job_submission_name,
                 job_submissions_table.c.job_submission_owner_email,
+                job_submissions_table.c.job_script_id,
                 job_scripts_table.c.job_script_name,
                 applications_table.c.application_name,
             ]
@@ -313,7 +315,26 @@ async def job_submissions_agent_pending(
     )
     logger.trace(f"query = {render_sql(query)}")
     rows = await database.fetch_all(query)
-    return rows
+
+    try:
+        response = [
+            PendingJobSubmission(
+                **row,
+                job_script_data_as_string=s3man_jobscripts[str(row.get("job_script_id"))],
+            )
+            for row in rows
+        ]
+    except KeyError:
+        list_ids = (str(row.get("job_script_id")) for row in rows)
+        missing_ids = {id for id in list_ids if id not in s3man_jobscripts}
+        message = f"JobScript file(s) not found, the missing ids are: {', '.join(missing_ids)}"
+        logger.error(message)
+        raise HTTPException(
+            status_code=status.HTTP_404_NOT_FOUND,
+            detail=message,
+        )
+
+    return response
 
 
 @router.put(
diff --git a/jobbergate-api/jobbergate_api/apps/job_submissions/schemas.py b/jobbergate-api/jobbergate_api/apps/job_submissions/schemas.py
index c59b5f57..cb09c1c6 100644
--- a/jobbergate-api/jobbergate_api/apps/job_submissions/schemas.py
+++ b/jobbergate-api/jobbergate_api/apps/job_submissions/schemas.py
@@ -126,6 +126,7 @@ class PendingJobSubmission(BaseModel, extra=Extra.ignore):
     job_submission_owner_email: str
     execution_directory: Optional[Path]
     job_script_name: str
+    job_script_data_as_string: str
     application_name: str
 
 


Test patch:
diff --git a/jobbergate-api/jobbergate_api/tests/apps/job_submissions/test_routers.py b/jobbergate-api/jobbergate_api/tests/apps/job_submissions/test_routers.py
index 0e482269..c219c1ee 100644
--- a/jobbergate-api/jobbergate_api/tests/apps/job_submissions/test_routers.py
+++ b/jobbergate-api/jobbergate_api/tests/apps/job_submissions/test_routers.py
@@ -2,6 +2,7 @@
 Tests for the /job-submissions/ endpoint.
 """
 import pathlib
+from unittest import mock
 
 import pytest
 from fastapi import status
@@ -1305,7 +1306,8 @@ async def test_job_submissions_agent_pending__success(
     This test proves that GET /job-submissions/agent/pending returns the correct job_submissions for the agent
     making the request. We show this by asserting that the job_submissions returned in the response are
     only job_submissions with a ``client_id`` that matches the ``client_id`` found in the request's
-    token payload.
+    token payload. We also make sure that the response includes job_script_data_as_string,
+    as it is fundamental for the integration with the agent.
     """
     inserted_application_id = await database.execute(
         query=applications_table.insert(),
@@ -1315,6 +1317,8 @@ async def test_job_submissions_agent_pending__success(
         query=job_scripts_table.insert(),
         values=fill_job_script_data(application_id=inserted_application_id),
     )
+    DUMMY_JOB_SCRIPT = "print(__name__)"
+
     await database.execute_many(
         query=job_submissions_table.insert(),
         values=fill_all_job_submission_data(
@@ -1357,12 +1361,96 @@ async def test_job_submissions_agent_pending__success(
         Permissions.JOB_SUBMISSIONS_VIEW,
         client_id="dummy-client",
     )
-    response = await client.get("/jobbergate/job-submissions/agent/pending")
+    with mock.patch(
+        "jobbergate_api.apps.job_submissions.routers.s3man_jobscripts",
+        {str(inserted_job_script_id): DUMMY_JOB_SCRIPT},
+    ) as mocked_s3:
+        response = await client.get("/jobbergate/job-submissions/agent/pending")
+
     assert response.status_code == status.HTTP_200_OK
+    assert str(inserted_job_script_id) in mocked_s3
 
     data = response.json()
     assert sorted(d["job_submission_name"] for d in data) == ["sub1", "sub4"]
     assert sorted(d["job_submission_owner_email"] for d in data) == ["email1@dummy.com", "email4@dummy.com"]
+    assert [d["job_script_data_as_string"] for d in data] == [DUMMY_JOB_SCRIPT] * 2
+
+
+@pytest.mark.asyncio
+async def test_job_submissions_agent_pending__missing_job_script_file(
+    client,
+    fill_application_data,
+    fill_job_script_data,
+    fill_all_job_submission_data,
+    inject_security_header,
+):
+    """
+    Test GET /job-submissions/agent/pending returns a 404.
+
+    This test proves that GET /job-submissions/agent/pending returns a 404 status
+    if any of the job-script files is not found. It also makes sure that the missing
+    id(s) are included in the response to support debugging.
+    """
+    inserted_application_id = await database.execute(
+        query=applications_table.insert(),
+        values=fill_application_data(),
+    )
+    inserted_job_script_id = await database.execute(
+        query=job_scripts_table.insert(),
+        values=fill_job_script_data(application_id=inserted_application_id),
+    )
+
+    await database.execute_many(
+        query=job_submissions_table.insert(),
+        values=fill_all_job_submission_data(
+            dict(
+                job_script_id=inserted_job_script_id,
+                job_submission_name="sub1",
+                job_submission_owner_email="email1@dummy.com",
+                status=JobSubmissionStatus.CREATED,
+                client_id="dummy-client",
+            ),
+            dict(
+                job_script_id=inserted_job_script_id,
+                job_submission_name="sub2",
+                job_submission_owner_email="email2@dummy.com",
+                status=JobSubmissionStatus.COMPLETED,
+                client_id="dummy-client",
+            ),
+            dict(
+                job_script_id=inserted_job_script_id,
+                job_submission_name="sub3",
+                job_submission_owner_email="email3@dummy.com",
+                status=JobSubmissionStatus.CREATED,
+                client_id="silly-client",
+            ),
+            dict(
+                job_script_id=inserted_job_script_id,
+                job_submission_name="sub4",
+                job_submission_owner_email="email4@dummy.com",
+                status=JobSubmissionStatus.CREATED,
+                client_id="dummy-client",
+            ),
+        ),
+    )
+
+    count = await database.fetch_all("SELECT COUNT(*) FROM job_submissions")
+    assert count[0][0] == 4
+
+    inject_security_header(
+        "who@cares.com",
+        Permissions.JOB_SUBMISSIONS_VIEW,
+        client_id="dummy-client",
+    )
+    with mock.patch(
+        "jobbergate_api.apps.job_submissions.routers.s3man_jobscripts",
+        {},
+    ) as mocked_s3:
+        response = await client.get("/jobbergate/job-submissions/agent/pending")
+
+    assert str(inserted_job_script_id) not in mocked_s3
+    assert response.status_code == status.HTTP_404_NOT_FOUND
+    assert f"JobScript file(s) not found, the missing ids are: {inserted_job_script_id}" in response.text
 
 
 @pytest.mark.asyncio