[GPCAPIM-359] Sort out env vars

.github/actions/start-app/action.yaml

@@ -4,7 +4,8 @@
   deploy-command:
     description: "Command to start app"
     required: false
-    default: "make deploy"
+    # TODO: rename this make deploy-ci?
+    default: "make deploy-dev"
   health-path:
     description: "Health check path"
     required: false

.github/instructions/copilot-instructions.md

@@ -8,7 +8,7 @@ This repository is for handling HTTP requests from "Consumer systems" and forwar
 
 We use other NHSE services to assist in the validation and processing of the requests including PDS FHIR API for obtaining GP practice codes for the patient, SDS FHIR API for obtaining the "Provider system" details of that GP practice and Healthcare Worker FHIR API for obtaining details of the requesting practitioner using the "Consumer System" that will then be added to the forwarded request.
 
-`make deploy` will build and start a container running Gateway API at `localhost:5000`.
+`make deploy-dev` will build and start a container running Gateway API at `localhost:5000`.
 
 After deploying the container locally, `make test` will run all tests and capture their coverage. Note: env variables control the use of stubs for the PDS FHIR API, SDS FHIR API, Healthcare Worker FHIR API and Provider system services.
 

.github/workflows/preview-env.yml

@@ -248,6 +248,7 @@
           --services ${{ steps.tf-output.outputs.ecs_service }} \
           --region ${{ env.AWS_REGION }}
 
+      # TODO: We don't need these anymore, as we are testing against the proxy.
       - name: Get mTLS certs for testing
         if: github.event.action != 'closed'
         id: mtls-certs

.github/workflows/stage-2-test.yaml

@@ -3,9 +3,9 @@ name: "Test stage"
 env:
   BASE_URL: "http://localhost:5000"
   HOST: "localhost"
-  STUB_SDS: "true"
-  STUB_PDS: "true"
-  STUB_PROVIDER: "true"
+  SDS_URL: "stub"
+  PDS_URL: "stub"
+  PROVIDER_URL: "stub"
 
 on:
   workflow_call:

.vscode/cspell-dictionary.txt

@@ -73,6 +73,7 @@ NOSONAR
 NPFIT
 ONESHELL
 opencollection
+orangebox
 pipefail
 PIPX
 pkce

Makefile

@@ -66,29 +66,20 @@ build: build-gateway-api # Build the project artefact @Pipeline
 publish: # Publish the project artefact @Pipeline
 	# TODO [GPCAPIM-283]:  Implement the artefact publishing step
 
-deploy: clean build # Deploy the project artefact to the target environment @Pipeline
+deploy: clean build # Build project artefact and deploy locally @Pipeline
 	@$(docker) network inspect gateway-local >/dev/null 2>&1 || $(docker) network create gateway-local
-	# Build up list of environment variables to pass to the container
-	@ENVIRONMENT_STRING="" ; \
-	if [[ -n "$${STUB_PROVIDER}" ]]; then \
-		ENVIRONMENT_STRING="$${ENVIRONMENT_STRING} -e STUB_PROVIDER=$${STUB_PROVIDER}" ; \
-	fi ; \
-	if [[ -n "$${STUB_PDS}" ]]; then \
-		ENVIRONMENT_STRING="$${ENVIRONMENT_STRING} -e STUB_PDS=$${STUB_PDS}" ; \
-	fi ; \
-	if [[ -n "$${STUB_SDS}" ]]; then \
-		ENVIRONMENT_STRING="$${ENVIRONMENT_STRING} -e STUB_SDS=$${STUB_SDS}" ; \
-	fi ; \
-	if [[ -n "$${CDG_DEBUG}" ]]; then \
-		ENVIRONMENT_STRING="$${ENVIRONMENT_STRING} -e CDG_DEBUG=$${CDG_DEBUG}" ; \
-	fi ; \
+	@echo "Using environment variables from .env"
+	@cat .env
 	if [[ -n "$${IN_BUILD_CONTAINER}" ]]; then \
 		echo "Starting using local docker network ..." ; \
-		$(docker) run --platform linux/amd64 --name gateway-api -p 5000:8080 --network gateway-local $${ENVIRONMENT_STRING} -d ${IMAGE_NAME} ; \
+		$(docker) run --platform linux/amd64 --name gateway-api -p 5000:8080 --network gateway-local --env-file .env -d ${IMAGE_NAME} ; \
 	else \
-		$(docker) run --platform linux/amd64 --name gateway-api -p 5000:8080 $${ENVIRONMENT_STRING} -d ${IMAGE_NAME} ; \
+		$(docker) run --platform linux/amd64 --name gateway-api -p 5000:8080 --env-file .env -d ${IMAGE_NAME} ; \
 	fi
 
+deploy-%: # Build project artefact and deploy locally as specified environment - mandatory: name=[name of the environment, e.g. 'dev'] @Pipeline
+	make env-$* deploy
+
 clean:: stop # Clean-up project resources (main) @Operations
 	@echo "Removing Gateway API container..."
 	@$(docker) rm gateway-api || echo "No Gateway API container currently exists."

README.md

@@ -129,9 +129,10 @@ The project uses `make` targets to build, deploy, and manage the application. Ru
 | --- | --- |
 | `make dependencies` | Install all project dependencies via Poetry |
 | `make build` | Type-check, package, and build the Docker image |
-| `make deploy` | Build and start the Gateway API container at `localhost:5000` |
+| `make deploy` | Build and start the Gateway API container using the environment variables defined in `.env` |
 | `make clean` | Stop and remove the Gateway API container |
 | `make config` | Configure the development environment |
+| `make env` | Create a `.env` to be consumed when starting the app, e.g. `make deploy` |
 
 ### API Endpoints
 
@@ -152,10 +153,10 @@ The full API schema is defined in [gateway-api/openapi.yaml](gateway-api/openapi
 | `HOST` | hostname portion of `BASE_URL` |
 | `FLASK_HOST` | Host the Flask app binds to |
 | `FLASK_PORT` | Port the Flask app listens on |
-| `STUB_PDS` | `true`, use the stubs/stubs/pds/stub.py to return stubbed responses for PDS FHIR API; otherwise, not. |
-| `STUB_SDS` | `true`, use the stubs/stubs/sds/stub.py to return stubbed responses for SDS FHIR API; otherwise, not. |
-| `STUB_PROVIDER` | `true`, use the stubs/stubs/provider/stub.py to return stubbed responses for the provider system; otherwise, not. |
-| `CDG_DEBUG` | `true`, Return additional debug information when the call to the GP provider returns an error. Note if set true causes the unit tests to fail, because expected return values are changed. |
+| `PDS_URL` | The URL for the PDS FHIR API; set as `stub` to use development stub. |
+| `SDS_URL` | The URL for the SDS FHIR API; set as `stub` to use development stub. |
+| `PROVIDER_URL` | The URL for the GP Provider; set as `stub` to use development stub. |
+| `CDG_DEBUG` | `true`, return additional debug information when the call to the GP provider returns an error. |
 
 Environment variables also control whether stubs are used in place of the real PDS, SDS, and Provider services during local development.
 

gateway-api/pyproject.toml

@@ -23,7 +23,7 @@ packages = [{include = "gateway_api", from = "src"},
 
 [tool.coverage.run]
 relative_files = true
-omit = ["*/tests/*", "*/features/*", "*/test_*.py"]
+omit = ["*/tests/*", "*/features/*", "*/test_*.py", "*/conftest.py"]
 
 [tool.coverage.paths]
 source = [

gateway-api/src/gateway_api/app.py

@@ -1,5 +1,7 @@
 import os
 import traceback
+from collections.abc import Callable
+from typing import Any
 
 from flask import Flask, Request, request
 from flask.wrappers import Response
@@ -15,20 +17,31 @@
 app.logger.setLevel("INFO")
 
 
-def get_app_host() -> str:
-    host = os.getenv("FLASK_HOST")
-    if host is None:
-        raise RuntimeError("FLASK_HOST environment variable is not set.")
-    print(f"Starting Flask app on host: {host}")
-    return host
+def start_app(app: Flask) -> None:
+    log_env_vars(app)
+    configure_app(app)
+    log_starting_app(app)
+    app.run(host=app.config["FLASK_HOST"], port=app.config["FLASK_PORT"])
 
 
-def get_app_port() -> int:
-    port = os.getenv("FLASK_PORT")
-    if port is None:
-        raise RuntimeError("FLASK_PORT environment variable is not set.")
-    print(f"Starting Flask app on port: {port}")
-    return int(port)
+def configure_app(app: Flask) -> None:
+    config = {
+        "FLASK_HOST": get_env_var("FLASK_HOST", str),
+        "FLASK_PORT": get_env_var("FLASK_PORT", int),
+        "PDS_URL": get_env_var("PDS_URL", str),
+        "SDS_URL": get_env_var("SDS_URL", str),
+    }
+    app.config.update(config)
+
+
+def get_env_var(name: str, loader: Callable[[str], Any]) -> Any:
+    value = os.getenv(name)
+    if value is None:
+        raise RuntimeError(f"{name} environment variable is not set.")
+    try:
+        return loader(value)
+    except Exception as e:
+        raise RuntimeError(f"Error loading {name} environment variable: {e}") from e
 
 
 def log_request_received(request: Request) -> None:
@@ -51,14 +64,35 @@ def log_error(error: AbstractCDGError) -> None:
     app.logger.error(log_details)
 
 
+def log_env_vars(app: Flask) -> None:
+    log_details = {
+        "description": "Initializing Flask app",
+        "env_vars": os.environ.items(),
+    }
+    app.logger.info(log_details)
+
+
+def log_starting_app(app: Flask) -> None:
+    log_details = {
+        "description": "Starting Flask app",
+        "host": app.config["FLASK_HOST"],
+        "port": app.config["FLASK_PORT"],
+        "pds_base_url": app.config["PDS_URL"],
+        "sds_base_url": app.config["SDS_URL"],
+    }
+    app.logger.info(log_details)
+
+
 @app.route("/patient/$gpc.getstructuredrecord", methods=["POST"])
 def get_structured_record() -> Response:
     log_request_received(request)
     response = GetStructuredRecordResponse()
     response.mirror_headers(request)
     try:
         get_structured_record_request = GetStructuredRecordRequest(request)
-        controller = Controller()
+        controller = Controller(
+            pds_base_url=app.config["PDS_URL"], sds_base_url=app.config["SDS_URL"]
+        )
         provider_response = controller.run(request=get_structured_record_request)
         response.add_provider_response(provider_response)
     except AbstractCDGError as e:
@@ -86,6 +120,4 @@ def health_check() -> dict[str, str]:
 
 
 if __name__ == "__main__":
-    host, port = get_app_host(), get_app_port()
-    print(f"Version: {os.getenv('COMMIT_VERSION')}")
-    app.run(host=host, port=port)
+    start_app(app)

gateway-api/src/gateway_api/conftest.py

@@ -1,7 +1,10 @@
 """Pytest configuration and shared fixtures for gateway API tests."""
 
 import json
+import os
+from collections.abc import Mapping
 from dataclasses import dataclass
+from types import TracebackType
 from typing import Any
 
 import pytest
@@ -13,6 +16,36 @@
 
 from gateway_api.clinical_jwt import JWT
 
+# TODO: Do this better.
+os.environ["PDS_URL"] = "stub"
+os.environ["PROVIDER_URL"] = "not-stub"
+os.environ["SDS_URL"] = "stub"
+
+
+class NewEnvVars:
+    def __init__(self, new_env_vars: Mapping[str, str | None]) -> None:
+        self.new_env_vars = new_env_vars
+        self.original_env_vars = {}
+        for key in new_env_vars:
+            if key in os.environ:
+                self.original_env_vars[key] = os.environ[key]
+
+    def __enter__(self) -> "NewEnvVars":
+        for key, value in self.new_env_vars.items():
+            if value is None and key in os.environ:
+                del os.environ[key]
+            elif value is not None:
+                os.environ[key] = value
+        return self
+
+    def __exit__(
+        self,
+        _type: type[BaseException] | None,
+        _value: BaseException | None,
+        _traceback: TracebackType | None,
+    ) -> None:
+        os.environ.update(self.original_env_vars)
+
 
 @dataclass
 class FakeResponse:

gateway-api/src/gateway_api/controller.py

@@ -27,8 +27,8 @@ class Controller:
 
     def __init__(
         self,
-        pds_base_url: str = PdsClient.SANDBOX_URL,
-        sds_base_url: str = SdsClient.SANDBOX_URL,
+        pds_base_url: str,
+        sds_base_url: str,
         timeout: int = 10,
     ) -> None:
         """

gateway-api/src/gateway_api/pds/client.py

@@ -20,7 +20,6 @@
 
 import os
 import uuid
-from collections.abc import Callable
 
 import requests
 from fhir.r4 import Patient
@@ -29,18 +28,17 @@
 from gateway_api.common.error import PdsRequestFailedError
 
 # TODO [GPCAPIM-359]: Once stub servers/containers made for PDS, SDS and provider
-#       we should remove the STUB_PDS environment variable and just
+#       we should remove the PDS_URL environment variable and just
 #       use the stub client
-STUB_PDS = os.environ.get("STUB_PDS", "false").lower() == "true"
+STUB_PDS = os.environ["PDS_URL"].lower() == "stub"
 
-get: Callable[..., requests.Response]
 if not STUB_PDS:
-    get = requests.get
+    from requests import get
 else:
     from stubs.pds.stub import PdsFhirApiStub
 
     pds = PdsFhirApiStub()
-    get = pds.get
+    get = pds.get  # type: ignore
 
 
 class PdsClient:
@@ -67,15 +65,10 @@
             print(result)
     """
 
-    # URLs for different PDS environments. Requires authentication to use live.
-    SANDBOX_URL = "https://sandbox.api.service.nhs.uk/personal-demographics/FHIR/R4"
-    INT_URL = "https://int.api.service.nhs.uk/personal-demographics/FHIR/R4"
-    PROD_URL = "https://api.service.nhs.uk/personal-demographics/FHIR/R4"
-
     def __init__(
         self,
         auth_token: str,
-        base_url: str = SANDBOX_URL,
+        base_url: str,
         timeout: int = 10,
         ignore_dates: bool = False,
     ) -> None:
@@ -84,6 +77,8 @@
         self.timeout = timeout
         self.ignore_dates = ignore_dates
 
+        # TODO: Add logging to show stub behaviour
+
     def _build_headers(
         self,
         request_id: str | None = None,
@@ -123,7 +118,8 @@
 
         url = f"{self.base_url}/Patient/{nhs_number}"
 
-        # This normally calls requests.get, but if STUB_PDS is set it uses the stub.
+        # This normally calls requests.get, but if PDS_URL is set it uses the stub.
+        # TODO: Log request to confirm client behaviour
         response = get(
             url,
             headers=headers,
@@ -133,6 +129,7 @@
 
         try:
             response.raise_for_status()
+            # TODO: Log response to confirm stub behaviour
         except requests.HTTPError as err:
             raise PdsRequestFailedError(error_reason=err.response.reason) from err