From b512cdaf0661588d3e0166d0d27740947a6c17d7 Mon Sep 17 00:00:00 2001 From: Joel Klinger Date: Thu, 31 Oct 2024 16:11:28 +0000 Subject: [PATCH 01/20] [feature/PI-575-remove_implicit_ok] remove implicit ok --- src/api/createCpmProduct/tests/test_index.py | 2 - .../tests/v1/test_index_v1.py | 3 - src/api/deleteCpmProduct/src/v1/steps.py | 4 +- src/api/readCpmProduct/tests/test_index.py | 3 - src/api/readDevice/tests/test_index.py | 4 -- .../tests/test_index.py | 4 -- src/api/readProductTeam/tests/test_index.py | 3 - src/api/readQuestionnaire/src/v1/steps.py | 6 +- src/api/status/index.py | 2 +- src/api/status/tests/test_index.py | 2 - .../steps/tests/test_mock_requests.py | 1 - .../api_utils/api_step_chain/__init__.py | 2 +- .../domain/response/aws_lambda_response.py | 4 -- src/layers/domain/response/render_response.py | 59 +++++++------------ src/layers/domain/response/steps.py | 4 +- .../tests/test_aws_lambda_response.py | 1 - .../response/tests/test_render_response.py | 24 ++++---- src/layers/domain/response/validators.py | 6 +- src/test_helpers/response_assertions.py | 2 +- 19 files changed, 46 insertions(+), 90 deletions(-) diff --git a/src/api/createCpmProduct/tests/test_index.py b/src/api/createCpmProduct/tests/test_index.py index 0a24a9353..6b50bdc81 100644 --- a/src/api/createCpmProduct/tests/test_index.py +++ b/src/api/createCpmProduct/tests/test_index.py @@ -78,7 +78,6 @@ def test_index(version): "Content-Length": str(len(expected_body)), "Content-Type": "application/json", "Version": version, - "Location": "FOO", }, } _response_assertions( @@ -149,7 +148,6 @@ def test_index_no_such_product_team(version): "Content-Length": str(len(expected_result)), "Content-Type": "application/json", "Version": version, - "Location": None, }, } _response_assertions( diff --git a/src/api/createProductTeam/tests/v1/test_index_v1.py b/src/api/createProductTeam/tests/v1/test_index_v1.py index 85583e3c4..48640fcb5 100644 --- a/src/api/createProductTeam/tests/v1/test_index_v1.py +++ b/src/api/createProductTeam/tests/v1/test_index_v1.py @@ -58,7 +58,6 @@ def test_index(version): "Content-Length": str(len(expected_body)), "Content-Type": "application/json", "Version": version, - "Location": None, }, } _response_assertions( @@ -107,7 +106,6 @@ def test_index_bad_payload(version): "Content-Length": str(len(expected_body)), "Content-Type": "application/json", "Version": version, - "Location": None, }, } _response_assertions( @@ -158,7 +156,6 @@ def test_index(version): "Content-Length": str(len(expected_body)), "Content-Type": "application/json", "Version": version, - "Location": None, }, } _response_assertions( diff --git a/src/api/deleteCpmProduct/src/v1/steps.py b/src/api/deleteCpmProduct/src/v1/steps.py index 3a5cf2a1c..5eba28bc3 100644 --- a/src/api/deleteCpmProduct/src/v1/steps.py +++ b/src/api/deleteCpmProduct/src/v1/steps.py @@ -14,8 +14,8 @@ def delete_product(data, cache) -> CpmProduct: return product_repo.write(product) -def set_http_status(data, cache) -> int: - return HTTPStatus.NO_CONTENT +def set_http_status(data, cache) -> tuple[int, None]: + return HTTPStatus.NO_CONTENT, None steps = [ diff --git a/src/api/readCpmProduct/tests/test_index.py b/src/api/readCpmProduct/tests/test_index.py index df45b1305..829afc181 100644 --- a/src/api/readCpmProduct/tests/test_index.py +++ b/src/api/readCpmProduct/tests/test_index.py @@ -82,7 +82,6 @@ def test_index(version): expected_headers = { "Content-Type": "application/json", "Version": version, - "Location": None, } # Check response headers @@ -146,7 +145,6 @@ def test_index_no_such_cpm_product(version): "Content-Length": str(len(expected_result)), "Content-Type": "application/json", "Version": version, - "Location": None, }, } _response_assertions( @@ -199,7 +197,6 @@ def test_index_no_such_product_team(version): "Content-Length": str(len(expected_result)), "Content-Type": "application/json", "Version": version, - "Location": None, }, } _response_assertions( diff --git a/src/api/readDevice/tests/test_index.py b/src/api/readDevice/tests/test_index.py index 8f8a31e57..4ed6bfeba 100644 --- a/src/api/readDevice/tests/test_index.py +++ b/src/api/readDevice/tests/test_index.py @@ -93,7 +93,6 @@ def test_index(version): expected_headers = { "Content-Type": "application/json", "Version": version, - "Location": None, } # Check response headers @@ -168,7 +167,6 @@ def test_index_no_such_device(version): "Content-Length": str(len(expected_result)), "Content-Type": "application/json", "Version": version, - "Location": None, }, } _response_assertions( @@ -232,7 +230,6 @@ def test_index_no_such_product(version): "Content-Length": str(len(expected_result)), "Content-Type": "application/json", "Version": version, - "Location": None, }, } _response_assertions( @@ -286,7 +283,6 @@ def test_index_no_such_product_team(version): "Content-Length": str(len(expected_result)), "Content-Type": "application/json", "Version": version, - "Location": None, }, } _response_assertions( diff --git a/src/api/readDeviceReferenceData/tests/test_index.py b/src/api/readDeviceReferenceData/tests/test_index.py index 7dc739a81..7157538eb 100644 --- a/src/api/readDeviceReferenceData/tests/test_index.py +++ b/src/api/readDeviceReferenceData/tests/test_index.py @@ -99,7 +99,6 @@ def test_index(version): expected_headers = { "Content-Type": "application/json", "Version": version, - "Location": None, } # Check response headers @@ -174,7 +173,6 @@ def test_index_no_such_device_reference_data(version): "Content-Length": str(len(expected_result)), "Content-Type": "application/json", "Version": version, - "Location": None, }, } _response_assertions( @@ -238,7 +236,6 @@ def test_index_no_such_product(version): "Content-Length": str(len(expected_result)), "Content-Type": "application/json", "Version": version, - "Location": None, }, } _response_assertions( @@ -292,7 +289,6 @@ def test_index_no_such_product_team(version): "Content-Length": str(len(expected_result)), "Content-Type": "application/json", "Version": version, - "Location": None, }, } _response_assertions( diff --git a/src/api/readProductTeam/tests/test_index.py b/src/api/readProductTeam/tests/test_index.py index 0f851a952..3207f55f8 100644 --- a/src/api/readProductTeam/tests/test_index.py +++ b/src/api/readProductTeam/tests/test_index.py @@ -71,7 +71,6 @@ def test_index(version): "Content-Length": str(len(expected_result)), "Content-Type": "application/json", "Version": version, - "Location": None, }, } _response_assertions( @@ -121,7 +120,6 @@ def test_index_no_such_product_team(version, product_id): "Content-Length": str(len(expected_result)), "Content-Type": "application/json", "Version": version, - "Location": None, }, } _response_assertions( @@ -185,7 +183,6 @@ def test_index_by_alias(version): "Content-Length": str(len(expected_result)), "Content-Type": "application/json", "Version": version, - "Location": None, }, } _response_assertions( diff --git a/src/api/readQuestionnaire/src/v1/steps.py b/src/api/readQuestionnaire/src/v1/steps.py index d9e04015d..4f76eb609 100644 --- a/src/api/readQuestionnaire/src/v1/steps.py +++ b/src/api/readQuestionnaire/src/v1/steps.py @@ -1,3 +1,5 @@ +from http import HTTPStatus + from aws_lambda_powertools.utilities.data_classes import APIGatewayProxyEvent from domain.core.questionnaire.v3 import Questionnaire from domain.repository.questionnaire_repository.v2 import QuestionnaireRepository @@ -18,9 +20,9 @@ def read_questionnaire(data, cache) -> Questionnaire: return repo.read(name=path_params.questionnaire_id) -def questionnaire_to_dict(data, cache) -> dict: +def questionnaire_to_dict(data, cache) -> tuple[int, dict]: questionnaire: Questionnaire = data[read_questionnaire] - return questionnaire.state() + return HTTPStatus.OK, questionnaire.state() steps = [ diff --git a/src/api/status/index.py b/src/api/status/index.py index 2c001bfa8..e4b3aade3 100644 --- a/src/api/status/index.py +++ b/src/api/status/index.py @@ -26,5 +26,5 @@ def handler(event: dict, context=None): api_chain.run(cache=cache, init=event) response_chain = StepChain(step_chain=post_steps, step_decorators=step_decorators) - response_chain.run(init=(api_chain.result, None, None)) + response_chain.run(init=(api_chain.result, None)) return response_chain.result diff --git a/src/api/status/tests/test_index.py b/src/api/status/tests/test_index.py index 708cad241..40e513150 100644 --- a/src/api/status/tests/test_index.py +++ b/src/api/status/tests/test_index.py @@ -55,7 +55,6 @@ def test_index(): "Content-Length": str(len(expected_result)), "Content-Type": "application/json", "Version": "null", - "Location": None, }, } _response_assertions( @@ -96,7 +95,6 @@ def test_index_not_ok(): "Content-Length": str(len(expected_body)), "Content-Type": "application/json", "Version": "null", - "Location": None, }, } _response_assertions( diff --git a/src/api/tests/feature_tests/steps/tests/test_mock_requests.py b/src/api/tests/feature_tests/steps/tests/test_mock_requests.py index 8872f19f3..289285efa 100644 --- a/src/api/tests/feature_tests/steps/tests/test_mock_requests.py +++ b/src/api/tests/feature_tests/steps/tests/test_mock_requests.py @@ -39,7 +39,6 @@ def test__mock_requests(): "Content-Length": str(len(response_body)), "Content-Type": "application/json", "Version": "1", - "Location": None, "Host": None, }, "status_code": 200, diff --git a/src/layers/api_utils/api_step_chain/__init__.py b/src/layers/api_utils/api_step_chain/__init__.py index 15203c020..a02586259 100644 --- a/src/layers/api_utils/api_step_chain/__init__.py +++ b/src/layers/api_utils/api_step_chain/__init__.py @@ -45,5 +45,5 @@ def execute_step_chain( response_chain = StepChain( step_chain=response_steps, step_decorators=STEP_DECORATORS ) - response_chain.run(init=(result, version, None)) + response_chain.run(init=(result, version)) return response_chain.result diff --git a/src/layers/domain/response/aws_lambda_response.py b/src/layers/domain/response/aws_lambda_response.py index 25a86e865..080607e46 100644 --- a/src/layers/domain/response/aws_lambda_response.py +++ b/src/layers/domain/response/aws_lambda_response.py @@ -10,7 +10,6 @@ class AwsLambdaResponseHeaders(BaseModel): ) content_length: str = Field(alias="Content-Length", regex=r"^[0-9][0-9]*$") version: str = Field(alias="Version", regex=r"^(null)|([1-9][0-9]*)$") - location: str = Field(default=None, alias="Location") host: str = Field(default=None, alias="Host") class Config: @@ -24,7 +23,6 @@ class AwsLambdaResponse(BaseModel): statusCode: HTTPStatus body: str = Field(min_length=0, default="") version: None | str = Field(exclude=True) - location: None | str = Field(exclude=True, default=None) headers: AwsLambdaResponseHeaders = None @validator("headers", always=True) @@ -33,11 +31,9 @@ def generate_response_headers(cls, headers, values): return headers body: str = values["body"] version: None | str = values["version"] - location: None | str = values.get("location") headers = AwsLambdaResponseHeaders( content_length=len(body), version="null" if version is None else version, - location=location, host="foo.co.uk", ) return headers diff --git a/src/layers/domain/response/render_response.py b/src/layers/domain/response/render_response.py index 49bf78291..051eb7ba4 100644 --- a/src/layers/domain/response/render_response.py +++ b/src/layers/domain/response/render_response.py @@ -3,7 +3,6 @@ from domain.response.error_response import ErrorResponse from domain.response.response_matrix import http_status_from_exception -from nhs_context_logging import app_logger from pydantic import ValidationError from .aws_lambda_response import AwsLambdaResponse @@ -14,46 +13,30 @@ ) -def render_response[ - JsonSerialisable -]( - response: JsonSerialisable | HTTPStatus | Exception, - id: str = None, # Deprecated: remove when FHIR is removed - version: str = None, - location: str = None, -) -> AwsLambdaResponse: - if id is None: - id = app_logger.service_name +def _exception_to_response_tuple(exception: Exception) -> tuple[HTTPStatus, object]: + _exception = validate_exception(exception) + exception_renderer = ( + ErrorResponse.from_validation_error + if isinstance(exception, ValidationError) + else ErrorResponse.from_exception + ) + outcome = exception_renderer(exception=_exception).dict() + http_status = http_status_from_exception(exception=_exception) + return http_status, outcome - if isinstance(response, HTTPStatus): - # Convert response to an Exception if it is an invalid http status - response = validate_http_status_response(response) - elif not isinstance(response, (HTTPStatus, Exception)): - # Convert response to an Exception if it isn't JSON serialisable - response = validate_json_serialisable_response(response) +def render_response( + response: Exception | tuple[HTTPStatus, object], version: str = None +) -> AwsLambdaResponse: if isinstance(response, Exception): - response = validate_exception(response) - - if response == HTTPStatus.NO_CONTENT: - http_status = response - outcome = None - elif isinstance(response, ValidationError): - # Implicit failure from ValidationError - outcome = ErrorResponse.from_validation_error(exception=response).dict() - http_status = http_status_from_exception(exception=response) - elif isinstance(response, Exception): - # Implicit failure from all other Exceptions - outcome = ErrorResponse.from_exception(exception=response).dict() - http_status = http_status_from_exception(exception=response) - elif isinstance(response, tuple): - http_status, outcome = response + http_status, outcome = _exception_to_response_tuple(exception=response) else: - # Implicit success (e.g. SEARCH, READ operations) - http_status = HTTPStatus.OK - outcome = response + http_status, outcome = response + try: + validate_http_status_response(http_status=http_status) + validate_json_serialisable_response(item=outcome) + except Exception as exception: + http_status, outcome = _exception_to_response_tuple(exception=exception) body = json.dumps(outcome) if outcome is not None else "" - return AwsLambdaResponse( - statusCode=http_status, body=body, version=version, location=location - ) + return AwsLambdaResponse(statusCode=http_status, body=body, version=version) diff --git a/src/layers/domain/response/steps.py b/src/layers/domain/response/steps.py index ea29f67c1..e03245e8c 100644 --- a/src/layers/domain/response/steps.py +++ b/src/layers/domain/response/steps.py @@ -6,8 +6,8 @@ def render_response(data, cache) -> dict: - result, version, location = data[StepChain.INIT] - response = _render_response(response=result, version=version, location=location) + result, version = data[StepChain.INIT] + response = _render_response(response=result, version=version) return response.dict() diff --git a/src/layers/domain/response/tests/test_aws_lambda_response.py b/src/layers/domain/response/tests/test_aws_lambda_response.py index 3c0b351a6..504e72cb3 100644 --- a/src/layers/domain/response/tests/test_aws_lambda_response.py +++ b/src/layers/domain/response/tests/test_aws_lambda_response.py @@ -11,7 +11,6 @@ def test_aws_lambda_response(aws_lambda_response: AwsLambdaResponse): "headers": { "Content-Type": "application/json", "Content-Length": f"{len(aws_lambda_response.body)}", - "Location": None, "Version": "null", "Host": "foo.co.uk", }, diff --git a/src/layers/domain/response/tests/test_render_response.py b/src/layers/domain/response/tests/test_render_response.py index 4c878eab1..e8973e56e 100644 --- a/src/layers/domain/response/tests/test_render_response.py +++ b/src/layers/domain/response/tests/test_render_response.py @@ -15,8 +15,10 @@ NON_SUCCESS_STATUSES = set(HTTPStatus._member_map_.values()) - SUCCESS_STATUSES -def test_render_response_of_json_serialisable(): - aws_lambda_response = render_response(response={"dict": "of things"}) +def test_render_response_of_json_serialisable_ok(): + aws_lambda_response = render_response( + response=(HTTPStatus.OK, {"dict": "of things"}) + ) expected = { "statusCode": HTTPStatus.OK, "body": '{"dict": "of things"}', @@ -37,9 +39,7 @@ def test_render_response_of_json_serialisable(): def test_render_response_of_success_http_status_created(): expected_body = json.dumps({"foo": "bar"}) - aws_lambda_response = render_response( - response=(HTTPStatus.CREATED, {"foo": "bar"}), - ) + aws_lambda_response = render_response(response=(HTTPStatus.CREATED, {"foo": "bar"})) expected = { "statusCode": 201, @@ -75,7 +75,7 @@ def test_render_response_of_non_success_http_status(http_status: HTTPStatus): ], } ) - aws_lambda_response = render_response(response=http_status, id="foo") + aws_lambda_response = render_response(response=(http_status, "some response")) expected = { "statusCode": HTTPStatus.INTERNAL_SERVER_ERROR, @@ -107,7 +107,7 @@ def test_render_response_of_non_json_serialisable(): } ) - aws_lambda_response = render_response(object(), id="foo") + aws_lambda_response = render_response((HTTPStatus.OK, object())) expected = { "statusCode": HTTPStatus.INTERNAL_SERVER_ERROR, "body": expected_body, @@ -135,7 +135,7 @@ def test_render_response_of_non_json_serialisable(): ], ) def test_render_response_of_json_serialisable(response, expected_body): - aws_lambda_response = render_response(response, id="foo") + aws_lambda_response = render_response((HTTPStatus.OK, response)) expected = { "statusCode": HTTPStatus.OK, "body": expected_body, @@ -154,7 +154,7 @@ def test_render_response_of_json_serialisable(response, expected_body): def test_render_response_of_blank_exception(): - aws_lambda_response = render_response(response=Exception(), id="foo") + aws_lambda_response = render_response(response=Exception()) expected_body = json.dumps( { "errors": [ @@ -183,7 +183,7 @@ def test_render_response_of_blank_exception(): def test_render_response_of_general_exception(): - aws_lambda_response = render_response(response=Exception("oops"), id="foo") + aws_lambda_response = render_response(response=Exception("oops")) expected_body = json.dumps( { "errors": [ @@ -223,7 +223,7 @@ def test_render_response_of_general_validation_error(): } validation_error = _get_validation_error(model_inputs) - aws_lambda_response = render_response(response=validation_error, id="foo") + aws_lambda_response = render_response(response=validation_error) expected_body = { "errors": [ { @@ -252,7 +252,7 @@ def test_render_response_of_general_validation_error(): def test_render_response_of_internal_validation_error(): validation_error = _get_inbound_validation_error() - aws_lambda_response = render_response(response=validation_error, id="foo") + aws_lambda_response = render_response(response=validation_error) expected_body = { "errors": [ { diff --git a/src/layers/domain/response/validators.py b/src/layers/domain/response/validators.py index 3314678aa..64ae926c4 100644 --- a/src/layers/domain/response/validators.py +++ b/src/layers/domain/response/validators.py @@ -28,16 +28,14 @@ def __init__(self, exception): def validate_http_status_response(http_status: HTTPStatus): if http_status not in SUCCESS_STATUSES: - return UnexpectedHttpStatus(http_status=http_status) - return http_status + raise UnexpectedHttpStatus(http_status=http_status) def validate_json_serialisable_response(item): try: json.dumps(item) except Exception as exception: - return NotJsonSerialisable(str(exception)) - return item + raise NotJsonSerialisable(str(exception)) def validate_exception(exception): diff --git a/src/test_helpers/response_assertions.py b/src/test_helpers/response_assertions.py index fbcc257a5..6ba3da6d7 100644 --- a/src/test_helpers/response_assertions.py +++ b/src/test_helpers/response_assertions.py @@ -14,5 +14,5 @@ def _response_assertions( assert key in result if isinstance(value, dict): _response_assertions(result=result.get(key, {}), expected=value) - elif key not in {"Location", "headers"}: + elif key != "headers": _assert_key_value(key, value, result, check_body, check_content_length) From a90ae92d4023439cb50b1d2557f9440136d01e9a Mon Sep 17 00:00:00 2001 From: Joel Klinger Date: Thu, 31 Oct 2024 15:57:44 +0000 Subject: [PATCH 02/20] [feature/PI-293-e2e_with_apigee] e2e with apigee --- .github/workflows/pull-requests.yml | 9 +++++- scripts/builder/build.mk | 2 +- scripts/infrastructure/apigee.mk | 31 ++++++++++++------- scripts/infrastructure/apigee/apigee.sh | 10 ++---- scripts/infrastructure/apigee/proxygen.sh | 1 - scripts/test/test.mk | 3 +- src/api/tests/feature_tests/environment.py | 21 +++++-------- .../features/status.success.feature | 18 ----------- .../tests/feature_tests/steps/assertion.py | 6 ++++ src/api/tests/feature_tests/steps/context.py | 2 +- src/api/tests/feature_tests/steps/requests.py | 26 ++++++++++++++-- src/api/tests/feature_tests/steps/steps.py | 5 +-- 12 files changed, 72 insertions(+), 62 deletions(-) delete mode 100644 src/api/tests/feature_tests/features/status.success.feature diff --git a/.github/workflows/pull-requests.yml b/.github/workflows/pull-requests.yml index 6e01d37f3..b132620b8 100644 --- a/.github/workflows/pull-requests.yml +++ b/.github/workflows/pull-requests.yml @@ -218,6 +218,7 @@ jobs: test--unit, test--feature--local, terraform-head-build, + apigee--attach-product, ] runs-on: [self-hosted, ci] strategy: @@ -276,7 +277,13 @@ jobs: requires-aws: true apigee--detach-product: - needs: [build-head, test--smoke, apigee--attach-product] + needs: + [ + build-head, + test--smoke, + apigee--attach-product, + test--feature--integration, + ] runs-on: [self-hosted, ci] if: ${{ needs.apigee--attach-product.result == 'success' }} steps: diff --git a/scripts/builder/build.mk b/scripts/builder/build.mk index 2e893777b..3797f79e4 100644 --- a/scripts/builder/build.mk +++ b/scripts/builder/build.mk @@ -6,7 +6,7 @@ POSTMAN_COLLECTION = $(CURDIR)/src/api/tests/feature_tests/postman-collection.js TOOL_VERSIONS_COPY = $(TIMESTAMP_DIR)/tool-versions.copy POETRY_LOCK = $(CURDIR)/poetry.lock INIT_TIMESTAMP = $(CURDIR)/.timestamp/init.timestamp -SRC_FILES = $(shell find src -type f -name "*.py" -not -path "*/test_*" -not -path "*/fhir/r4/strict_models.py" -not -path "*/fhir/r4/models.py") +SRC_FILES = $(shell find src -type f -name "*.py" -not -path "*/feature_tests/*" -not -path "*/test_*" -not -path "*/fhir/r4/strict_models.py" -not -path "*/fhir/r4/models.py") THIRD_PARTY_DIST = $(CURDIR)/src/layers/third_party/dist SWAGGER_DIST = $(CURDIR)/infrastructure/swagger/dist SWAGGER_PUBLIC = $(SWAGGER_DIST)/public/swagger.yaml diff --git a/scripts/infrastructure/apigee.mk b/scripts/infrastructure/apigee.mk index 36aac74ea..0387f9ed1 100644 --- a/scripts/infrastructure/apigee.mk +++ b/scripts/infrastructure/apigee.mk @@ -3,13 +3,14 @@ APIGEE_CONFIG_PATH = $(CURDIR)/infrastructure/apigee APIGEE_TIMESTAMP = $(TIMESTAMP_DIR)/.apigee.stamp PROXYGEN_TIMESTAMP = $(TIMESTAMP_DIR)/.proxygen.stamp +PROXYGEN_PRODUCT_TIMESTAMP = $(TIMESTAMP_DIR)/.proxygen-product.stamp SWAGGER_APIGEE = $(SWAGGER_DIST)/apigee/swagger.yaml WORKSPACE_OUTPUT_JSON = $(CURDIR)/infrastructure/terraform/per_workspace/output.json ENVIRONMENT_MAPPING_YAML = $(CURDIR)/infrastructure/apigee/environment_mapping.yaml STAGE_MAPPING_YAML = $(CURDIR)/infrastructure/apigee/stage_mapping.yaml -apigee--deploy: $(PROXYGEN_TIMESTAMP) +apigee--deploy: aws--login $(PROXYGEN_TIMESTAMP) apigee--delete: aws--login @@ -26,15 +27,7 @@ apigee--delete: aws--login apigee--clean: [[ -f $(PROXYGEN_TIMESTAMP) ]] && rm $(PROXYGEN_TIMESTAMP) || : -apigee--attach-product: aws--login - WORKSPACE_OUTPUT_JSON=$(WORKSPACE_OUTPUT_JSON) \ - ENVIRONMENT_MAPPING_YAML=$(ENVIRONMENT_MAPPING_YAML) \ - STAGE_MAPPING_YAML=$(STAGE_MAPPING_YAML) \ - APIGEE_CONFIG_PATH=$(APIGEE_CONFIG_PATH) \ - AWS_ACCESS_KEY_ID=$(AWS_ACCESS_KEY_ID) \ - AWS_SECRET_ACCESS_KEY=$(AWS_SECRET_ACCESS_KEY) \ - AWS_SESSION_TOKEN=$(AWS_SESSION_TOKEN) \ - bash $(PATH_TO_INFRASTRUCTURE)/apigee/apigee.sh attach_product $(PERSISTENT_ENVIRONMENT_BUILD) +apigee--attach-product: aws--login $(PROXYGEN_PRODUCT_TIMESTAMP) apigee--detach-product: aws--login WORKSPACE_OUTPUT_JSON=$(WORKSPACE_OUTPUT_JSON) \ @@ -45,8 +38,10 @@ apigee--detach-product: aws--login AWS_SECRET_ACCESS_KEY=$(AWS_SECRET_ACCESS_KEY) \ AWS_SESSION_TOKEN=$(AWS_SESSION_TOKEN) \ bash $(PATH_TO_INFRASTRUCTURE)/apigee/apigee.sh detach_product $(PERSISTENT_ENVIRONMENT_BUILD) + [[ -f $(PROXYGEN_PRODUCT_TIMESTAMP) ]] && rm $(PROXYGEN_PRODUCT_TIMESTAMP) || : -$(PROXYGEN_TIMESTAMP): aws--login $(SWAGGER_APIGEE) $(WORKSPACE_OUTPUT_JSON) + +$(PROXYGEN_TIMESTAMP): $(SWAGGER_APIGEE) $(WORKSPACE_OUTPUT_JSON) [[ -f $(PROXYGEN_TIMESTAMP) ]] && rm $(PROXYGEN_TIMESTAMP) || : WORKSPACE_OUTPUT_JSON=$(WORKSPACE_OUTPUT_JSON) \ @@ -60,3 +55,17 @@ $(PROXYGEN_TIMESTAMP): aws--login $(SWAGGER_APIGEE) $(WORKSPACE_OUTPUT_JSON) bash $(PATH_TO_INFRASTRUCTURE)/apigee/proxygen.sh generate_proxy $(PERSISTENT_ENVIRONMENT_BUILD) touch $(PROXYGEN_TIMESTAMP) + + +$(PROXYGEN_PRODUCT_TIMESTAMP): $(PROXYGEN_TIMESTAMP) + [[ -f $(PROXYGEN_PRODUCT_TIMESTAMP) ]] && rm $(PROXYGEN_PRODUCT_TIMESTAMP) || : + + WORKSPACE_OUTPUT_JSON=$(WORKSPACE_OUTPUT_JSON) \ + ENVIRONMENT_MAPPING_YAML=$(ENVIRONMENT_MAPPING_YAML) \ + STAGE_MAPPING_YAML=$(STAGE_MAPPING_YAML) \ + APIGEE_CONFIG_PATH=$(APIGEE_CONFIG_PATH) \ + AWS_ACCESS_KEY_ID=$(AWS_ACCESS_KEY_ID) \ + AWS_SECRET_ACCESS_KEY=$(AWS_SECRET_ACCESS_KEY) \ + AWS_SESSION_TOKEN=$(AWS_SESSION_TOKEN) \ + bash $(PATH_TO_INFRASTRUCTURE)/apigee/apigee.sh attach_product $(PERSISTENT_ENVIRONMENT_BUILD) + touch $(PROXYGEN_PRODUCT_TIMESTAMP) diff --git a/scripts/infrastructure/apigee/apigee.sh b/scripts/infrastructure/apigee/apigee.sh index 4385b54c1..a6d8cb917 100644 --- a/scripts/infrastructure/apigee/apigee.sh +++ b/scripts/infrastructure/apigee/apigee.sh @@ -146,17 +146,14 @@ function attach_product(){ token=$(echo "$token_response" | jq -r '.pytest_nhsd_apim_token') url=https://api.enterprise.apigee.com/v1/organizations/$_org_name/developers/$email_that_owns_app/apps/$app_name/keys/$client_id - echo "$url" - add_product_response=$(curl -X POST \ + add_product_response=$(curl --retry 100 -sS -X POST \ $url \ -H "Authorization: Bearer $token" \ -H "Content-type:application/json" \ -d "{\"apiProducts\": [\"$_product_name\"]}") status=$(echo "$add_product_response" | jq -r '.status') - echo "$status" - if [ -z "$status" ] || [ "$status" != "approved" ]; then echo "Failed to add product to the app" echo "Response: $add_product_response" @@ -232,17 +229,14 @@ function detach_product(){ token=$(echo "$token_response" | jq -r '.pytest_nhsd_apim_token') url=https://api.enterprise.apigee.com/v1/organizations/$_org_name/developers/$email_that_owns_app/apps/$app_name/keys/$client_id/apiproducts/$_product_name - echo "$url" - detach_product_response=$(curl -X DELETE \ + detach_product_response=$(curl -sS -X DELETE \ $url \ -H "Authorization: Bearer $token" \ -H "Content-type:application/json" \ -d "{\"apiProducts\": [\"$_product_name\"]}") status=$(echo "$detach_product_response" | jq -r '.status') - echo "$status" - if [ -z "$status" ] || [ "$status" != "approved" ]; then echo "Failed to remove product from the app" echo "Response: $detach_product_response" diff --git a/scripts/infrastructure/apigee/proxygen.sh b/scripts/infrastructure/apigee/proxygen.sh index 6c8261b8b..9d778e869 100644 --- a/scripts/infrastructure/apigee/proxygen.sh +++ b/scripts/infrastructure/apigee/proxygen.sh @@ -4,7 +4,6 @@ PATH_TO_HERE="scripts/infrastructure/apigee" APIGEE_DEPLOYMENT_ROLE="NHSDeploymentRole" API_NAME="connecting-party-manager" PERSISTENT_ENVIRONMENT_BUILD="${2:-false}" -echo "PERSISTENT_ENVIRONMENT_BUILD is: $PERSISTENT_ENVIRONMENT_BUILD" if [[ -z ${WORKSPACE_OUTPUT_JSON} ]]; then diff --git a/scripts/test/test.mk b/scripts/test/test.mk index 2a86ff380..e59589e0f 100644 --- a/scripts/test/test.mk +++ b/scripts/test/test.mk @@ -7,6 +7,7 @@ USE_CPM_PROD ?= FALSE TEST_COUNT = COMPARISON_ENV ?= local RUN_SPEEDTEST = ?= FALSE +PROXYGEN_PRODUCT_TIMESTAMP = $(TIMESTAMP_DIR)/.proxygen-product.stamp _pytest: AWS_DEFAULT_REGION=$(AWS_DEFAULT_REGION) AWS_ACCESS_KEY_ID=$(AWS_ACCESS_KEY_ID) AWS_SECRET_ACCESS_KEY=$(AWS_SECRET_ACCESS_KEY) AWS_SESSION_TOKEN=$(AWS_SESSION_TOKEN) poetry run python -m pytest $(PYTEST_FLAGS) $(_INTERNAL_FLAGS) $(_CACHE_CLEAR) @@ -32,7 +33,7 @@ test--smoke: aws--login ## Run end-to-end smoke tests (pytest) test--%--rerun: ## Rerun failed integration or unit (pytest) tests $(MAKE) test--$* _INTERNAL_FLAGS="--last-failed --last-failed-no-failures none" _CACHE_CLEAR=$(_CACHE_CLEAR) -test--feature--integration: aws--login ## Run integration feature (gherkin) tests +test--feature--integration: aws--login $(PROXYGEN_PRODUCT_TIMESTAMP) ## Run integration feature (gherkin) tests $(MAKE) _behave _INTERNAL_FLAGS="--define='test_mode=integration' $(_INTERNAL_FLAGS)" AWS_ACCESS_KEY_ID=$(AWS_ACCESS_KEY_ID) AWS_SECRET_ACCESS_KEY=$(AWS_SECRET_ACCESS_KEY) AWS_SESSION_TOKEN=$(AWS_SESSION_TOKEN) test--feature--local: _behave ## Run local feature (gherkin) tests diff --git a/src/api/tests/feature_tests/environment.py b/src/api/tests/feature_tests/environment.py index 62f3c77ed..f02f994c6 100644 --- a/src/api/tests/feature_tests/environment.py +++ b/src/api/tests/feature_tests/environment.py @@ -3,7 +3,7 @@ from behave import use_fixture from behave.model import Feature, Scenario, Step -from event.aws.client import dynamodb_client, secretsmanager_client +from event.aws.client import dynamodb_client from api.tests.feature_tests.feature_test_helpers import TestMode from api.tests.feature_tests.steps.context import Context @@ -17,6 +17,7 @@ PostmanCollection, PostmanItem, ) +from api.tests.smoke_tests.utils import get_app_key, get_base_url from test_helpers.aws_session import aws_session from test_helpers.dynamodb import clear_dynamodb_table from test_helpers.terraform import read_terraform_output @@ -37,32 +38,26 @@ def before_all(context: Context): context.workspace_type = "" context.environment = "" context.notes = {} + context.api_key = "" # pragma: allowlist secret if context.test_mode is TestMode.INTEGRATION: context.table_name = read_terraform_output("dynamodb_table_name.value") - context.base_url = read_terraform_output("certificate_domain_name.value") + "/" context.workspace_type = read_terraform_output("workspace_type.value") context.workspace = read_terraform_output("workspace.value") context.environment = read_terraform_output("environment.value") + context.base_url = ( + get_base_url(workspace=context.workspace, environment=context.environment) + + "/" + ) context.session = aws_session with context.session(): - client = secretsmanager_client() - if context.workspace_type in {"LOCAL", "CI"}: - secret_name = f"{context.environment}-apigee-cpm-apikey" # pragma: allowlist secret - else: - secret_name = ( - f"{context.workspace}-apigee-cpm-apikey" # pragma: allowlist secret - ) - - response = client.get_secret_value(SecretId=secret_name) - context.apikey = response["SecretString"] + context.api_key = get_app_key(environment=context.environment) if context.test_mode is TestMode.LOCAL: use_fixture(mock_environment, context=context, table_name=context.table_name) use_fixture(mock_dynamodb, context=context, table_name=context.table_name) use_fixture(mock_requests, context=context) - context.apikey = "mock" # pragma: allowlist secret def before_feature(context: Context, feature: Feature): diff --git a/src/api/tests/feature_tests/features/status.success.feature b/src/api/tests/feature_tests/features/status.success.feature deleted file mode 100644 index b1ad73ec8..000000000 --- a/src/api/tests/feature_tests/features/status.success.feature +++ /dev/null @@ -1,18 +0,0 @@ -Feature: Status - These scenarios demonstrate the expected behaviour of the status endpoint - - Background: - Given "default" request headers: - | name | value | - | Authorization | letmein | - - Scenario: Confirm Status endpoint is active - When I make a "GET" request with "default" headers to "_status" - Then I receive a status code "200" with body - | path | value | - | code | OK | - | message | Transaction successful | - And the response headers contain: - | name | value | - | Content-Type | application/json | - | Content-Length | 51 | diff --git a/src/api/tests/feature_tests/steps/assertion.py b/src/api/tests/feature_tests/steps/assertion.py index 086667a8d..291ec3f88 100644 --- a/src/api/tests/feature_tests/steps/assertion.py +++ b/src/api/tests/feature_tests/steps/assertion.py @@ -110,6 +110,12 @@ def assert_is_subset(expected: dict, received: dict, path=None): path = [""] for key, expected_value in expected.items(): + + # Sometimes AWS remaps keys, but not consistently + amzn_remapped_key = f"x-amzn-Remapped-{key}" + if key not in received and amzn_remapped_key in received: + key = amzn_remapped_key + # Key must exist assert key in received, error_message("Could not find key", key, "in", received) diff --git a/src/api/tests/feature_tests/steps/context.py b/src/api/tests/feature_tests/steps/context.py index 2f2ab9859..b8f645b33 100644 --- a/src/api/tests/feature_tests/steps/context.py +++ b/src/api/tests/feature_tests/steps/context.py @@ -23,7 +23,7 @@ class Context(BehaveContext): workspace: str = None environment: str = None workspace_type: str = None - apikey: str = None + api_key: str = None notes: dict[str, str] = None postman_collection: PostmanCollection = None diff --git a/src/api/tests/feature_tests/steps/requests.py b/src/api/tests/feature_tests/steps/requests.py index 1b786f704..9883a7ba4 100644 --- a/src/api/tests/feature_tests/steps/requests.py +++ b/src/api/tests/feature_tests/steps/requests.py @@ -1,4 +1,5 @@ import json as _json +import time from contextlib import contextmanager from dataclasses import dataclass from unittest import mock @@ -8,6 +9,7 @@ from domain.response.aws_lambda_response import AwsLambdaResponse from event.json import json_loads from requests import HTTPError, Response, request +from requests.exceptions import SSLError from api.tests.feature_tests.steps.data import DUMMY_CONTEXT from api.tests.feature_tests.steps.endpoint_lambda_mapping import ( @@ -33,6 +35,21 @@ def _parse_url(base_url: str, endpoint: str) -> str: return url +@contextmanager +def retry_on_ssl_error(sleep_time: int = 3, max_retries=5): + retries = 0 + while True: + try: + yield + except SSLError: + if retries == max_retries: + raise + time.sleep(sleep_time) + retries += 1 + finally: + break + + def make_request( base_url: str, http_method: str, @@ -44,9 +61,12 @@ def make_request( url = _parse_url(base_url=base_url, endpoint=endpoint) json = body if type(body) is dict else None data = None if type(body) is dict else body - response = request( - method=http_method, url=url, headers=headers, json=json, data=data - ) + + with retry_on_ssl_error(): + response = request( + method=http_method, url=url, headers=headers, json=json, data=data + ) + if raise_for_status: try: response.raise_for_status() diff --git a/src/api/tests/feature_tests/steps/steps.py b/src/api/tests/feature_tests/steps/steps.py index e7c218c7a..e0ef4aff4 100644 --- a/src/api/tests/feature_tests/steps/steps.py +++ b/src/api/tests/feature_tests/steps/steps.py @@ -23,10 +23,7 @@ @given('"{header_name}" request headers') def given_request_headers(context: Context, header_name: str): table_headers = parse_table(table=context.table, context=context) - apikey_header = { - "apikey": context.apikey - } # Hidden here because the value cant be written in the tests - context.headers[header_name] = {**table_headers, **apikey_header} + context.headers[header_name] = dict(**table_headers, apikey=context.api_key) @given( From c184d90391f3cfa78a43a4c912874d12f22aae75 Mon Sep 17 00:00:00 2001 From: Joel Klinger Date: Mon, 4 Nov 2024 10:55:12 +0000 Subject: [PATCH 03/20] [feature/PI-591-better_postman] better postman --- .gitignore | 1 + README.md | 55 +++++++++++++++++- src/api/tests/feature_tests/environment.py | 17 ++++++ src/api/tests/feature_tests/steps/context.py | 9 ++- .../tests/feature_tests/steps/decorators.py | 10 +++- src/api/tests/feature_tests/steps/postman.py | 45 +++++++++++++++ src/api/tests/feature_tests/steps/steps.py | 57 ++++++++++++++----- 7 files changed, 174 insertions(+), 20 deletions(-) diff --git a/.gitignore b/.gitignore index 0ee39a966..184f83671 100644 --- a/.gitignore +++ b/.gitignore @@ -78,6 +78,7 @@ dist openapi postman-collection.json +postman-environment.json token_cache.json cpm.cdx.json diff --git a/README.md b/README.md index f522aa6fc..7e7ddb9d0 100644 --- a/README.md +++ b/README.md @@ -10,9 +10,12 @@ 3. [AWS SSO Setup](#aws-sso-setup) 4. [Other helpful commands](#other-helpful-commands) 2. [Tests](#tests) -3. [Workflow](#workflow) -4. [Swagger](#swagger) -5. [ETL](#etl) +3. [pytest tests](#pytest-tests) +4. [End-to-End feature tests](#end-to-end-feature-tests) +5. [Generate the Feature Test Postman collection](#generate-the-feature-test-postman-collection) +6. [Workflow](#workflow) +7. [Swagger](#swagger) +8. [ETL](#etl) --- @@ -220,6 +223,52 @@ The VSCode settings for "Run and Debug" are also set up to run these tests if yo `make test--sds--matrix` is used for testing responses match in SDS FHIR between CPM and LDAP. You must provide `SDS_PROD_APIKEY` and `SDS_DEV_APIKEY`. There are 3 optional variables `USE_CPM_PROD`, defaults to `FALSE`, `COMPARISON_ENV`, defaults to `local` and `TEST_COUNT`, defaults to `10` and is the number of requests to make. Add `PYTEST_FLAGS='-sv'`. +### End-to-End feature tests + +The Feature tests use `behave` (rather than `pytest`) to execute cucumber/gherkin-style end-to-end tests of specific features, in principle +giving full end-to-end test coverage for API operations. + +Executing feature tests locally will give you a good idea whether you have implemented a well-behaved feature whilst in development (i.e. no need to redeploy whilst developing). + +Executing feature tests in integration mode will then give you confidence that the feature is ready to deploy in production, but has a much slower development cycle as it will need a full redeploy after codebase or infrastructure changes are implemented. + +#### Local + +To execute the feature tests entirely locally (executing lambdas directly, and otherwise mocking databases and responses to a high standard) you can do: + +```shell +make test--feature-local +``` + +If you would like to pass `behave` flags, e.g. to \[stop after the first failure\]: + +```shell +make test--feature-local BEHAVE_FLAGS="--stop" +``` + +#### Integration + +To execute the feature tests across the entire stack (including Apigee and AWS) you can do + +```shell +make test--feature-integration +``` + +### Generate the Feature Test Postman collection + +Our [end-to-end feature tests](#end-to-end-feature-tests) also generate working Postman collections. To generate the Postman collection quickly, without Apigee credentials, you can run the local feature tests. If you would like the Apigee +credentials generating then you should run the integration feature tests. The generated files are: + +- `src/api/tests/feature_tests/postman-collection.json` +- `src/api/tests/feature_tests/postman-environment.json` + +You can drag and drop `postman-collection.json` into the `Collections` tab on Postman, +and `postman-environment.json` on to the `Environments` tab (remember to activate it). If you generated these +with the local feature tests, then you will need to manually update the Apigee `baseUrl` and `apiKey` fields +in the environment (but these are filled out already if generated with the integration feature tests). + +💡 **The feature tests are only guaranteed to work out-of-the-box with an empty database** + ## Workflow In order to create new branches, use the commands listed below. Note that the commands will throw an error if diff --git a/src/api/tests/feature_tests/environment.py b/src/api/tests/feature_tests/environment.py index f02f994c6..ae352eeaa 100644 --- a/src/api/tests/feature_tests/environment.py +++ b/src/api/tests/feature_tests/environment.py @@ -15,6 +15,8 @@ from api.tests.feature_tests.steps.postman import ( BASE_URL, PostmanCollection, + PostmanEnvironment, + PostmanEnvironmentValue, PostmanItem, ) from api.tests.smoke_tests.utils import get_app_key, get_base_url @@ -39,6 +41,12 @@ def before_all(context: Context): context.environment = "" context.notes = {} context.api_key = "" # pragma: allowlist secret + context.postman_environment = PostmanEnvironment( + values=[ + PostmanEnvironmentValue(key="baseUrl", value=""), + PostmanEnvironmentValue(key="apiKey", value=""), + ] + ) if context.test_mode is TestMode.INTEGRATION: context.table_name = read_terraform_output("dynamodb_table_name.value") @@ -54,6 +62,13 @@ def before_all(context: Context): with context.session(): context.api_key = get_app_key(environment=context.environment) + context.postman_environment = PostmanEnvironment( + values=[ + PostmanEnvironmentValue(key="baseUrl", value=context.base_url), + PostmanEnvironmentValue(key="apiKey", value=context.api_key), + ] + ) + if context.test_mode is TestMode.LOCAL: use_fixture(mock_environment, context=context, table_name=context.table_name) use_fixture(mock_dynamodb, context=context, table_name=context.table_name) @@ -78,6 +93,7 @@ def before_scenario(context: Context, scenario: Scenario): def before_step(context: Context, step: Step): + context.postman_endpoint = None context.postman_step = PostmanItem( name=f"{step.keyword.lower().title()} {step.name}", item=None ) @@ -102,3 +118,4 @@ def after_feature(context: Context, feature: Feature): def after_all(context: Context): context.postman_collection.save(path=PATH_TO_HERE) + context.postman_environment.save(path=PATH_TO_HERE) diff --git a/src/api/tests/feature_tests/steps/context.py b/src/api/tests/feature_tests/steps/context.py index b8f645b33..8b360061a 100644 --- a/src/api/tests/feature_tests/steps/context.py +++ b/src/api/tests/feature_tests/steps/context.py @@ -7,7 +7,11 @@ from requests import Response from api.tests.feature_tests.feature_test_helpers import TestMode -from api.tests.feature_tests.steps.postman import PostmanCollection, PostmanItem +from api.tests.feature_tests.steps.postman import ( + PostmanCollection, + PostmanEnvironment, + PostmanItem, +) @dataclass @@ -25,8 +29,11 @@ class Context(BehaveContext): workspace_type: str = None api_key: str = None notes: dict[str, str] = None + postman_endpoint: str = None postman_collection: PostmanCollection = None postman_feature: PostmanItem = None postman_scenario: PostmanItem = None postman_step: PostmanItem = None + + postman_environment: PostmanEnvironment = None diff --git a/src/api/tests/feature_tests/steps/decorators.py b/src/api/tests/feature_tests/steps/decorators.py index d6ef543c2..b6c2f3176 100644 --- a/src/api/tests/feature_tests/steps/decorators.py +++ b/src/api/tests/feature_tests/steps/decorators.py @@ -5,6 +5,8 @@ from behave import then as _then from behave import when as _when +from api.tests.feature_tests.steps.context import Context +from api.tests.feature_tests.steps.postman import notes_as_postman_vars from api.tests.feature_tests.steps.table import expand_macro @@ -13,9 +15,11 @@ def _behave_decorator(description: str, behave_decorator: FunctionType) -> Funct def deco(fn): @wraps(fn) - def wrapper(context, **kwargs): - if "endpoint" in kwargs: - kwargs["endpoint"] = expand_macro(kwargs["endpoint"], context=context) + def wrapper(context: Context, **kwargs): + endpoint = kwargs.get("endpoint") + if endpoint: + context.postman_endpoint = notes_as_postman_vars(endpoint) + kwargs["endpoint"] = expand_macro(endpoint, context=context) return fn(context, **kwargs) return _deco(wrapper) diff --git a/src/api/tests/feature_tests/steps/postman.py b/src/api/tests/feature_tests/steps/postman.py index cb6558c21..86745dc9c 100644 --- a/src/api/tests/feature_tests/steps/postman.py +++ b/src/api/tests/feature_tests/steps/postman.py @@ -1,10 +1,13 @@ import json +import re from pathlib import Path from typing import Literal, Optional +from uuid import uuid4 from pydantic import BaseModel, Field POSTMAN_COLLECTION_FILENAME = "postman-collection.json" +POSTMAN_ENVIRONMENT_FILENAME = "postman-environment.json" BASE_URL = r"{{baseUrl}}/" OPTIONS = lambda: {"raw": {"language": "json"}} @@ -34,11 +37,23 @@ class PostmanRequest(BaseModel): url: Url +class Script(BaseModel): + exec: list[str] + type: Literal["text/javascript"] = Field(default="text/javascript") + packages: dict = Field(default_factory=dict) + + +class Event(BaseModel): + listen: Literal["test"] = Field(default="test") + script: Script + + class PostmanItem(BaseModel): name: str description: str = Field(default_factory=str) request: Optional[PostmanRequest] = None item: None | list["PostmanItem"] = Field(default_factory=list) + event: list[Event] = Field(default_factory=list) def __bool__(self): return bool(self.item) or bool(self.request) @@ -68,3 +83,33 @@ def save(self, path: Path): def __bool__(self): return bool(self.item) + + +class PostmanEnvironmentValue(BaseModel): + key: str + value: str + type: Literal["default"] = Field(default="default") + enabled: bool = Field(default=True) + + +class PostmanEnvironment(BaseModel): + id: str = Field(default_factory=lambda: str(uuid4())) + name: Literal["FeatureTestEnvironment"] = Field(default="FeatureTestEnvironment") + values: list[PostmanEnvironmentValue] + + def save(self, path: Path): + environment = self.dict(exclude_none=True, by_alias=True) + with open(path / POSTMAN_ENVIRONMENT_FILENAME, "w") as f: + json.dump(fp=f, obj=environment, indent=4) + + def __contains__(self, key: str): + return any(env.key == key for env in self.values) + + +def jsonpath_to_javascript_path(jsonpath: str): + jsonpath = jsonpath.lstrip("$") + return re.sub(r"\.(\d+)", r"[\1]", jsonpath) + + +def notes_as_postman_vars(text): + return re.sub(r"\$\{\s*note\((\w+)\)\s*\}", r"{{\1}}", text) diff --git a/src/api/tests/feature_tests/steps/steps.py b/src/api/tests/feature_tests/steps/steps.py index e0ef4aff4..0807e047f 100644 --- a/src/api/tests/feature_tests/steps/steps.py +++ b/src/api/tests/feature_tests/steps/steps.py @@ -10,7 +10,16 @@ ) from api.tests.feature_tests.steps.context import Context from api.tests.feature_tests.steps.decorators import given, then, when -from api.tests.feature_tests.steps.postman import Body, HeaderItem, PostmanRequest, Url +from api.tests.feature_tests.steps.postman import ( + Body, + Event, + HeaderItem, + PostmanEnvironmentValue, + PostmanRequest, + Script, + Url, + jsonpath_to_javascript_path, +) from api.tests.feature_tests.steps.requests import make_request from api.tests.feature_tests.steps.table import ( extract_from_response_by_jsonpath, @@ -43,13 +52,14 @@ def given_made_request( ) context.postman_step.request = PostmanRequest( url=Url( - raw=context.response.url, + raw=context.base_url + context.postman_endpoint, host=[context.base_url.rstrip("/")], - path=[endpoint], + path=[context.postman_endpoint], ), method=http_method, header=[ - HeaderItem(key=k, value=v) for k, v in context.headers[header_name].items() + HeaderItem(key=k, value="{{apiKey}}" if k == "apikey" else v) + for k, v in context.headers[header_name].items() ], body=Body(raw=json.dumps(body) if isinstance(body, dict) else body), ) @@ -70,13 +80,14 @@ def given_made_request( ) context.postman_step.request = PostmanRequest( url=Url( - raw=context.response.url, + raw=context.base_url + context.postman_endpoint, host=[context.base_url.rstrip("/")], - path=[endpoint], + path=[context.postman_endpoint], ), method=http_method, header=[ - HeaderItem(key=k, value=v) for k, v in context.headers[header_name].items() + HeaderItem(key=k, value="{{apiKey}}" if k == "apikey" else v) + for k, v in context.headers[header_name].items() ], ) @@ -101,13 +112,14 @@ def when_make_request( ) context.postman_step.request = PostmanRequest( url=Url( - raw=context.response.url, + raw=context.base_url + context.postman_endpoint, host=[context.base_url.rstrip("/")], - path=[endpoint], + path=[context.postman_endpoint], ), method=http_method, header=[ - HeaderItem(key=k, value=v) for k, v in context.headers[header_name].items() + HeaderItem(key=k, value="{{apiKey}}" if k == "apikey" else v) + for k, v in context.headers[header_name].items() ], body=Body(raw=json.dumps(body) if isinstance(body, dict) else body), ) @@ -125,13 +137,14 @@ def when_make_request( ) context.postman_step.request = PostmanRequest( url=Url( - raw=context.response.url, + raw=context.base_url + context.postman_endpoint, host=[context.base_url.rstrip("/")], - path=[endpoint], + path=[context.postman_endpoint], ), method=http_method, header=[ - HeaderItem(key=k, value=v) for k, v in context.headers[header_name].items() + HeaderItem(key=k, value="{{apiKey}}" if k == "apikey" else v) + for k, v in context.headers[header_name].items() ], ) @@ -257,3 +270,21 @@ def note_response_field(context: Context, jsonpath: str, alias: str): context.notes[alias] = extract_from_response_by_jsonpath( response=context.response.json(), jsonpath=jsonpath ) + + javascript_path = jsonpath_to_javascript_path(jsonpath) + context.postman_scenario.item[-1].event.append( + Event( + script=Script( + exec=[ + "if(pm.response.code >= 200 && pm.response.code < 300){", + f' pm.environment.set("{alias}", pm.response.json(){javascript_path})', + "}", + "", + ] + ) + ) + ) + if alias not in context.postman_environment: + context.postman_environment.values.append( + PostmanEnvironmentValue(key=alias, value="") + ) From f2a06429e2d325d77b8a3a90cf99e72bf9b5fee1 Mon Sep 17 00:00:00 2001 From: jameslinnell Date: Thu, 31 Oct 2024 15:21:27 +0000 Subject: [PATCH 04/20] Update dependencies in README --- README.md | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index f522aa6fc..5c9e36c03 100644 --- a/README.md +++ b/README.md @@ -22,16 +22,20 @@ We use `asdf` to fetch the required versions of prerequisite libraries instead of your system's default version. To get it up and running go to https://asdf-vm.com/guide/getting-started.html. You can check it installed properly by using the command `asdf --version`. -If you are using `pyenv` (you can check by typing `pyenv` and seeing whether it returns a nice list of commands) then you should run: - -``` -pyenv install $(cat .python-version) -``` +However, you will also need to install the `docker engine` separately Additionally you will need `wget` (doing `which wget` will return blank if not installed). Please Google "how to install wget on my operating system", if you don't already have this installed. +Update any dependencies on your system as required. + Otherwise `asdf` should do the work for you. +### Useful tools + +`VScode` is useful and we have a workspace file setup to allow easy integration + +`Postman` &/or `Newman` Feature tests create a postman.collection which can be used for manual testing. + ### Project build Do `make build` every time you would like to pick up and install new local/project dependencies and artifacts. This will always detect changes to: From efa8916ad4101999d70338b35faab4cf62124728 Mon Sep 17 00:00:00 2001 From: jameslinnell Date: Mon, 4 Nov 2024 10:39:30 +0000 Subject: [PATCH 05/20] Add delete scenario to smoke tests --- src/api/tests/smoke_tests/test_smoke.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/api/tests/smoke_tests/test_smoke.py b/src/api/tests/smoke_tests/test_smoke.py index ad11d06bb..c8348825c 100644 --- a/src/api/tests/smoke_tests/test_smoke.py +++ b/src/api/tests/smoke_tests/test_smoke.py @@ -53,6 +53,11 @@ def _request(base_url: str, headers: dict, path: str, method: str): "CreateCpmProductIncomingParams.foo: extra fields not permitted", ], ], + [ + "/ProductTeam/123/Product/abc", + "DELETE", + 404, + ], [ "/ProductTeam/123/Product/abc/DeviceReferenceData", "POST", From 10a1e89081702c971c29479231da13b862e5fa68 Mon Sep 17 00:00:00 2001 From: jameslinnell Date: Mon, 4 Nov 2024 14:10:22 +0000 Subject: [PATCH 06/20] Add Status and DELETE Product to smoke tests --- src/api/tests/smoke_tests/test_smoke.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/api/tests/smoke_tests/test_smoke.py b/src/api/tests/smoke_tests/test_smoke.py index c8348825c..41ceefeeb 100644 --- a/src/api/tests/smoke_tests/test_smoke.py +++ b/src/api/tests/smoke_tests/test_smoke.py @@ -22,6 +22,11 @@ def _request(base_url: str, headers: dict, path: str, method: str): @pytest.mark.parametrize( "request_details", [ + [ + "/_status", + "GET", + 200, + ], [ "/ProductTeam", "POST", From 3bd2bb4dac6433739f8bfe6c670fb2ace117126b Mon Sep 17 00:00:00 2001 From: jameslinnell Date: Mon, 4 Nov 2024 14:23:09 +0000 Subject: [PATCH 07/20] Add MhsMessageSet create to smoke tests --- src/api/tests/smoke_tests/test_smoke.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/api/tests/smoke_tests/test_smoke.py b/src/api/tests/smoke_tests/test_smoke.py index 41ceefeeb..68d86720e 100644 --- a/src/api/tests/smoke_tests/test_smoke.py +++ b/src/api/tests/smoke_tests/test_smoke.py @@ -73,6 +73,16 @@ def _request(base_url: str, headers: dict, path: str, method: str): "CreateDeviceReferenceDataIncomingParams.foo: extra fields not permitted", ], ], + [ + "/ProductTeam/123/Product/abc/DeviceReferenceData/MhsMessageSet", + "POST", + 400, + ["MISSING_VALUE", "VALIDATION_ERROR"], + [ + "CreateDeviceReferenceDataIncomingParams.name: field required", + "CreateDeviceReferenceDataIncomingParams.foo: extra fields not permitted", + ], + ], # ('/ProductTeam/123/Product/abc/Device', 'POST', 400, ['MISSING_VALUE', 'VALIDATION_ERROR']), [ "/ProductTeam/123", From 84c6db4d2dee2505bd53b8e07f2c4be54fba63ab Mon Sep 17 00:00:00 2001 From: Joel Klinger Date: Wed, 6 Nov 2024 14:27:12 +0000 Subject: [PATCH 08/20] [release/2024-11-06] create release --- CHANGELOG.md | 8 ++++++++ VERSION | 2 +- changelog/2024-11-06.md | 6 ++++++ pyproject.toml | 2 +- 4 files changed, 16 insertions(+), 2 deletions(-) create mode 100644 changelog/2024-11-06.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a429226c..be2ba2b04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ # Changelog +## 2024-11-06 +- [PI-593] Readme updates +- [PI-594] More smoke tests +- [PI-575] Remove implicit OK from responses +- [PI-558] Add search wrapper +- [PI-591] Better postman +- [PI-293] E2E tests with apigee + ## 2024-11-05 - [PI-585] PR/release CI builds in dev diff --git a/VERSION b/VERSION index fae25c101..91d72aef4 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2024.11.05 +2024.11.06 diff --git a/changelog/2024-11-06.md b/changelog/2024-11-06.md new file mode 100644 index 000000000..bfe055c9f --- /dev/null +++ b/changelog/2024-11-06.md @@ -0,0 +1,6 @@ +- [PI-593] Readme updates +- [PI-594] More smoke tests +- [PI-575] Remove implicit OK from responses +- [PI-558] Add search wrapper +- [PI-591] Better postman +- [PI-293] E2E tests with apigee diff --git a/pyproject.toml b/pyproject.toml index 2ed75f640..c8068f60e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "connecting-party-manager" -version = "2024.11.05" +version = "2024.11.06" description = "Repository for the Connecting Party Manager API and related services" authors = ["NHS England"] license = "LICENSE.md" From 3246fa80c2b2ed05d00cb0889011c1972a4bc660 Mon Sep 17 00:00:00 2001 From: jameslinnell Date: Wed, 30 Oct 2024 15:03:48 +0000 Subject: [PATCH 09/20] Add a search Results wrapper --- src/api/searchCpmProduct/src/v1/steps.py | 13 +- src/api/searchCpmProduct/tests/test_index.py | 81 ++++++++--- .../features/searchCpmProduct.success.feature | 130 +++++++++--------- src/api/tests/feature_tests/steps/steps.py | 7 + src/layers/domain/response/response_models.py | 13 ++ 5 files changed, 156 insertions(+), 88 deletions(-) create mode 100644 src/layers/domain/response/response_models.py diff --git a/src/api/searchCpmProduct/src/v1/steps.py b/src/api/searchCpmProduct/src/v1/steps.py index 56e785e0e..cf91a31e4 100644 --- a/src/api/searchCpmProduct/src/v1/steps.py +++ b/src/api/searchCpmProduct/src/v1/steps.py @@ -1,9 +1,9 @@ from http import HTTPStatus -from typing import List from aws_lambda_powertools.utilities.data_classes import APIGatewayProxyEvent from domain.repository.cpm_product_repository.v3 import CpmProductRepository from domain.repository.product_team_repository.v2 import ProductTeamRepository +from domain.response.response_models import SearchResponse from event.step_chain import StepChain @@ -20,7 +20,7 @@ def validate_product_team(data, cache) -> str: return product_team_repo.read(id=product_team_id) -def query_products(data, cache) -> tuple[HTTPStatus, List[dict]]: +def query_products(data, cache) -> list: product_team_id = data[parse_incoming_path_parameters] cpm_product_repo = CpmProductRepository( table_name=cache["DYNAMODB_TABLE"], dynamodb_client=cache["DYNAMODB_CLIENT"] @@ -28,11 +28,18 @@ def query_products(data, cache) -> tuple[HTTPStatus, List[dict]]: results = cpm_product_repo.query_products_by_product_team( product_team_id=product_team_id ) - return HTTPStatus.OK, [result.state() for result in results] + return results + + +def return_products(data, cache) -> tuple[HTTPStatus, str]: + products = data[query_products] + response = SearchResponse.from_models(products) + return HTTPStatus.OK, response.state() steps = [ parse_incoming_path_parameters, validate_product_team, query_products, + return_products, ] diff --git a/src/api/searchCpmProduct/tests/test_index.py b/src/api/searchCpmProduct/tests/test_index.py index 0847d0281..e1ac2dc75 100644 --- a/src/api/searchCpmProduct/tests/test_index.py +++ b/src/api/searchCpmProduct/tests/test_index.py @@ -1,3 +1,4 @@ +import json import os from unittest import mock @@ -9,6 +10,7 @@ from event.aws.client import dynamodb_client from event.json import json_loads +from test_helpers.response_assertions import _response_assertions from test_helpers.sample_data import CPM_PRODUCT_TEAM_NO_ID from test_helpers.terraform import read_terraform_output from test_helpers.validate_search_response import validate_product_result_body @@ -34,8 +36,14 @@ def _create_product(product, product_team): return cpmproduct +@pytest.mark.parametrize( + "version", + [ + "1", + ], +) @pytest.mark.integration -def test_no_results(): +def test_no_results(version): product_team = _create_org() product_team_id = product_team.id params = {"product_team_id": product_team_id} @@ -64,26 +72,40 @@ def test_no_results(): result = product_handler( event={ - "headers": {"version": 1}, + "headers": {"version": version}, "pathParameters": params, } ) result_body = json_loads(result["body"]) - assert result["statusCode"] == 200 - assert len(result_body) == 0 + expected_result = json.dumps({"results": []}) + expected = { + "statusCode": 200, + "body": expected_result, + "headers": { + "Content-Length": str(len(expected_result)), + "Content-Type": "application/json", + "Version": version, + "Location": None, + }, + } + _response_assertions( + result=result, expected=expected, check_body=True, check_content_length=True + ) + # assert result["statusCode"] == 200 + # assert len(result_body) == 0 @pytest.mark.integration @pytest.mark.parametrize( - "product", + "version,product", [ - {"name": "product-name-a"}, - {"name": "product-name-b"}, - {"name": "product-name-c"}, - {"name": "product-name-d"}, + ("1", {"product_name": "product-name-a"}), + ("1", {"product_name": "product-name-b"}), + ("1", {"product_name": "product-name-c"}), + ("1", {"product_name": "product-name-d"}), ], ) -def test_index(product): +def test_index(version, product): table_name = read_terraform_output("dynamodb_table_name.value") client = dynamodb_client() product_team = _create_org() @@ -116,20 +138,36 @@ def test_index(product): result = product_handler( event={ - "headers": {"version": 1}, + "headers": {"version": version}, "pathParameters": params, } ) - assert result["statusCode"] == 200 + expected_result = json.dumps({"results": [cpmproduct.state()]}) - result_body = json_loads(result["body"]) - assert isinstance(result_body, list) - validate_product_result_body(result_body, cpmproduct.state()) + expected = { + "statusCode": 200, + "body": expected_result, + "headers": { + "Content-Length": str(len(expected_result)), + "Content-Type": "application/json", + "Version": version, + "Location": None, + }, + } + _response_assertions( + result=result, expected=expected, check_body=True, check_content_length=True + ) @pytest.mark.integration -def test_index_no_such_product_team(): +@pytest.mark.parametrize( + "version", + [ + "1", + ], +) +def test_index_no_such_product_team(version): params = {"product_team_id": "123456"} table_name = read_terraform_output("dynamodb_table_name.value") client = dynamodb_client() @@ -154,7 +192,7 @@ def test_index_no_such_product_team(): result = product_handler( event={ - "headers": {"version": 1}, + "headers": {"version": version}, "pathParameters": params, } ) @@ -185,6 +223,7 @@ def test_index_no_such_product_team(): ], ) def test_index_multiple_returned(products): + version = 1 table_name = read_terraform_output("dynamodb_table_name.value") client = dynamodb_client() @@ -229,15 +268,15 @@ def test_index_multiple_returned(products): result = product_handler( event={ - "headers": {"version": 1}, + "headers": {"version": version}, "pathParameters": params, } ) assert result["statusCode"] == 200 - result_body = json_loads(result["body"]) - assert isinstance(result_body, list) + assert "results" in result_body + assert isinstance(result_body["results"], list) validate_product_result_body( - result_body, [cpm_product.state() for cpm_product in cpm_products] + result_body["results"], [cpm_product.state() for cpm_product in cpm_products] ) diff --git a/src/api/tests/feature_tests/features/searchCpmProduct.success.feature b/src/api/tests/feature_tests/features/searchCpmProduct.success.feature index 106d3533b..63173f308 100644 --- a/src/api/tests/feature_tests/features/searchCpmProduct.success.feature +++ b/src/api/tests/feature_tests/features/searchCpmProduct.success.feature @@ -16,11 +16,13 @@ Feature: Search CPM Products - success scenarios | keys.0.key_value | FOOBAR | Given I note the response field "$.id" as "product_team_id" When I make a "GET" request with "default" headers to "ProductTeam/${ note(product_team_id) }/Product" - Then I receive a status code "200" with an empty body + Then I receive a status code "200" with body + | path | value | + | results | [] | And the response headers contain: | name | value | | Content-Type | application/json | - | Content-Length | 2 | + | Content-Length | 15 | Scenario: Successfully search one CPM Product Given I have already made a "POST" request with "default" headers to "ProductTeam" with body: @@ -35,20 +37,20 @@ Feature: Search CPM Products - success scenarios | name | My Great CpmProduct | When I make a "GET" request with "default" headers to "ProductTeam/${ note(product_team_id) }/Product" Then I receive a status code "200" with body - | path | value | - | 0.id | << ignore >> | - | 0.product_team_id | ${ note(product_team_id) } | - | 0.name | My Great CpmProduct | - | 0.ods_code | F5H1R | - | 0.status | active | - | 0.keys | [] | - | 0.created_on | << ignore >> | - | 0.updated_on | << ignore >> | - | 0.deleted_on | << ignore >> | + | path | value | + | results.0.id | << ignore >> | + | results.0.product_team_id | ${ note(product_team_id) } | + | results.0.name | My Great CpmProduct | + | results.0.ods_code | F5H1R | + | results.0.status | active | + | results.0.keys | [] | + | results.0.created_on | << ignore >> | + | results.0.updated_on | << ignore >> | + | results.0.deleted_on | << ignore >> | And the response headers contain: | name | value | | Content-Type | application/json | - | Content-Length | 260 | + | Content-Length | 273 | Scenario: Successfully search more than one CPM Product Given I have already made a "POST" request with "default" headers to "ProductTeam" with body: @@ -68,39 +70,39 @@ Feature: Search CPM Products - success scenarios | path | value | | name | My Great Product 3 | When I make a "GET" request with "default" headers to "ProductTeam/${ note(product_team_id) }/Product" - Then I receive a status code "200" with a "product" search body response that contains - | path | value | - | 0.id | << ignore >> | - | 0.product_team_id | ${ note(product_team_id) } | - | 0.name | My Great Product 1 | - | 0.ods_code | F5H1R | - | 0.status | active | - | 0.keys | [] | - | 0.created_on | << ignore >> | - | 0.updated_on | << ignore >> | - | 0.deleted_on | << ignore >> | - | 1.id | << ignore >> | - | 1.product_team_id | ${ note(product_team_id) } | - | 1.name | My Great Product 2 | - | 1.ods_code | F5H1R | - | 1.status | active | - | 1.keys | [] | - | 1.created_on | << ignore >> | - | 1.updated_on | << ignore >> | - | 1.deleted_on | << ignore >> | - | 2.id | << ignore >> | - | 2.product_team_id | ${ note(product_team_id) } | - | 2.name | My Great Product 3 | - | 2.ods_code | F5H1R | - | 2.status | active | - | 2.keys | [] | - | 2.created_on | << ignore >> | - | 2.updated_on | << ignore >> | - | 2.deleted_on | << ignore >> | + Then I receive a status code "200" with body where "results" has a length of "3" + | path | value | + | results.0.id | << ignore >> | + | results.0.product_team_id | ${ note(product_team_id) } | + | results.0.name | My Great Product 1 | + | results.0.ods_code | F5H1R | + | results.0.status | active | + | results.0.keys | [] | + | results.0.created_on | << ignore >> | + | results.0.updated_on | << ignore >> | + | results.0.deleted_on | << ignore >> | + | results.1.id | << ignore >> | + | results.1.product_team_id | ${ note(product_team_id) } | + | results.1.name | My Great Product 2 | + | results.1.ods_code | F5H1R | + | results.1.status | active | + | results.1.keys | [] | + | results.1.created_on | << ignore >> | + | results.1.updated_on | << ignore >> | + | results.1.deleted_on | << ignore >> | + | results.2.id | << ignore >> | + | results.2.product_team_id | ${ note(product_team_id) } | + | results.2.name | My Great Product 3 | + | results.2.ods_code | F5H1R | + | results.2.status | active | + | results.2.keys | [] | + | results.2.created_on | << ignore >> | + | results.2.updated_on | << ignore >> | + | results.2.deleted_on | << ignore >> | And the response headers contain: | name | value | | Content-Type | application/json | - | Content-Length | 777 | + | Content-Length | 790 | Scenario: Deleted Products not returned in search Given I have already made a "POST" request with "default" headers to "ProductTeam" with body: @@ -124,27 +126,27 @@ Feature: Search CPM Products - success scenarios And I note the response field "$.id" as "product_id_3" And I have already made a "DELETE" request with "default" headers to "ProductTeam/${ note(product_team_id) }/Product/${ note(product_id_2) }" When I make a "GET" request with "default" headers to "ProductTeam/${ note(product_team_id) }/Product" - Then I receive a status code "200" with a "product" search body response that contains - | path | value | - | 0.id | ${ note(product_id_1) } | - | 0.product_team_id | ${ note(product_team_id) } | - | 0.name | My Great Product 1 | - | 0.ods_code | F5H1R | - | 0.status | active | - | 0.keys | [] | - | 0.created_on | << ignore >> | - | 0.updated_on | << ignore >> | - | 0.deleted_on | << ignore >> | - | 1.id | ${ note(product_id_3) } | - | 1.product_team_id | ${ note(product_team_id) } | - | 1.name | My Great Product 3 | - | 1.ods_code | F5H1R | - | 1.status | active | - | 1.keys | [] | - | 1.created_on | << ignore >> | - | 1.updated_on | << ignore >> | - | 1.deleted_on | << ignore >> | + Then I receive a status code "200" with body where "results" has a length of "2" + | path | value | + | results.0.id | ${ note(product_id_1) } | + | results.0.product_team_id | ${ note(product_team_id) } | + | results.0.name | My Great Product 1 | + | results.0.ods_code | F5H1R | + | results.0.status | active | + | results.0.keys | [] | + | results.0.created_on | << ignore >> | + | results.0.updated_on | << ignore >> | + | results.0.deleted_on | << ignore >> | + | results.1.id | ${ note(product_id_3) } | + | results.1.product_team_id | ${ note(product_team_id) } | + | results.1.name | My Great Product 3 | + | results.1.ods_code | F5H1R | + | results.1.status | active | + | results.1.keys | [] | + | results.1.created_on | << ignore >> | + | results.1.updated_on | << ignore >> | + | results.1.deleted_on | << ignore >> | And the response headers contain: | name | value | | Content-Type | application/json | - | Content-Length | 518 | + | Content-Length | 531 | diff --git a/src/api/tests/feature_tests/steps/steps.py b/src/api/tests/feature_tests/steps/steps.py index e7c218c7a..74f36bb92 100644 --- a/src/api/tests/feature_tests/steps/steps.py +++ b/src/api/tests/feature_tests/steps/steps.py @@ -175,6 +175,13 @@ def then_response(context: Context, status_code: str, list_to_check: str, count: except JSONDecodeError: response_body = context.response.text assert len(response_body[list_to_check]) == int(count) + if list_to_check == "results": + expected_body[list_to_check] = sorted( + expected_body[list_to_check], key=lambda x: x["name"] + ) + response_body[list_to_check] = sorted( + response_body[list_to_check], key=lambda x: x["name"] + ) assert_many( assertions=( assert_equal, diff --git a/src/layers/domain/response/response_models.py b/src/layers/domain/response/response_models.py new file mode 100644 index 000000000..0203303e8 --- /dev/null +++ b/src/layers/domain/response/response_models.py @@ -0,0 +1,13 @@ +from typing import Generic, List, TypeVar + +from domain.core.aggregate_root import AggregateRoot + +T = TypeVar("T", bound=AggregateRoot) + + +class SearchResponse(Generic[T], AggregateRoot): + results: List[T] + + @classmethod + def from_models(cls, models: List[T]) -> "SearchResponse[T]": + return cls(results=models) From 6f37cb9db08b8a8ba7d7b5dd50c360e0954ddd38 Mon Sep 17 00:00:00 2001 From: jameslinnell Date: Fri, 1 Nov 2024 09:33:52 +0000 Subject: [PATCH 10/20] Remove classmethod from Searchresponse --- src/api/searchCpmProduct/src/v1/steps.py | 2 +- src/layers/domain/response/response_models.py | 12 ++---------- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/src/api/searchCpmProduct/src/v1/steps.py b/src/api/searchCpmProduct/src/v1/steps.py index cf91a31e4..6531da1c8 100644 --- a/src/api/searchCpmProduct/src/v1/steps.py +++ b/src/api/searchCpmProduct/src/v1/steps.py @@ -33,7 +33,7 @@ def query_products(data, cache) -> list: def return_products(data, cache) -> tuple[HTTPStatus, str]: products = data[query_products] - response = SearchResponse.from_models(products) + response = SearchResponse(results=products) return HTTPStatus.OK, response.state() diff --git a/src/layers/domain/response/response_models.py b/src/layers/domain/response/response_models.py index 0203303e8..c8095a682 100644 --- a/src/layers/domain/response/response_models.py +++ b/src/layers/domain/response/response_models.py @@ -1,13 +1,5 @@ -from typing import Generic, List, TypeVar - from domain.core.aggregate_root import AggregateRoot -T = TypeVar("T", bound=AggregateRoot) - - -class SearchResponse(Generic[T], AggregateRoot): - results: List[T] - @classmethod - def from_models(cls, models: List[T]) -> "SearchResponse[T]": - return cls(results=models) +class SearchResponse[T](AggregateRoot): + results: list[T] From ee112c32d5a83e4f5de7935eea98669206d5db7b Mon Sep 17 00:00:00 2001 From: jameslinnell Date: Tue, 5 Nov 2024 14:03:01 +0000 Subject: [PATCH 11/20] Fix product_name --- src/api/searchCpmProduct/tests/test_index.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/api/searchCpmProduct/tests/test_index.py b/src/api/searchCpmProduct/tests/test_index.py index e1ac2dc75..b0a696cc3 100644 --- a/src/api/searchCpmProduct/tests/test_index.py +++ b/src/api/searchCpmProduct/tests/test_index.py @@ -76,7 +76,6 @@ def test_no_results(version): "pathParameters": params, } ) - result_body = json_loads(result["body"]) expected_result = json.dumps({"results": []}) expected = { "statusCode": 200, @@ -91,18 +90,16 @@ def test_no_results(version): _response_assertions( result=result, expected=expected, check_body=True, check_content_length=True ) - # assert result["statusCode"] == 200 - # assert len(result_body) == 0 @pytest.mark.integration @pytest.mark.parametrize( "version,product", [ - ("1", {"product_name": "product-name-a"}), - ("1", {"product_name": "product-name-b"}), - ("1", {"product_name": "product-name-c"}), - ("1", {"product_name": "product-name-d"}), + ("1", {"name": "product-name-a"}), + ("1", {"name": "product-name-b"}), + ("1", {"name": "product-name-c"}), + ("1", {"name": "product-name-d"}), ], ) def test_index(version, product): From 9e15a73791f450b9ff80666801d7eddee42390bf Mon Sep 17 00:00:00 2001 From: jameslinnell Date: Thu, 7 Nov 2024 11:30:09 +0000 Subject: [PATCH 12/20] Fix destroy script --- scripts/infrastructure/destroy/destroy-redundant-workspaces.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/scripts/infrastructure/destroy/destroy-redundant-workspaces.sh b/scripts/infrastructure/destroy/destroy-redundant-workspaces.sh index 060aa5d10..62b4db6d5 100644 --- a/scripts/infrastructure/destroy/destroy-redundant-workspaces.sh +++ b/scripts/infrastructure/destroy/destroy-redundant-workspaces.sh @@ -37,12 +37,11 @@ function _destroy_redundant_workspaces() { workspaces=$(aws s3 ls "$bucket" --no-paginate | awk '{print $NF}' | sed 's:/$::') # get JIRA ID from branch name + ENVIRONMENT="dev" if [[ $BRANCH_NAME =~ feature\/(PI-[0-9]+)[-_] ]]; then workspace_id="${BASH_REMATCH[1]}" - ENVIRONMENT="ref" elif [[ $BRANCH_NAME == *release/* ]]; then workspace_id="${BRANCH_NAME##*release/}" - ENVIRONMENT="dev" fi echo "The workspace ID is: $workspace_id" echo "Destroying workspaces in: $ENVIRONMENT" From d6d6d3918a728ae5b75754e32ad1cec573eab1e6 Mon Sep 17 00:00:00 2001 From: jameslinnell Date: Thu, 7 Nov 2024 12:15:39 +0000 Subject: [PATCH 13/20] Remove Location from searchCPMProducts --- src/api/searchCpmProduct/tests/test_index.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/api/searchCpmProduct/tests/test_index.py b/src/api/searchCpmProduct/tests/test_index.py index b0a696cc3..c5276b076 100644 --- a/src/api/searchCpmProduct/tests/test_index.py +++ b/src/api/searchCpmProduct/tests/test_index.py @@ -84,7 +84,6 @@ def test_no_results(version): "Content-Length": str(len(expected_result)), "Content-Type": "application/json", "Version": version, - "Location": None, }, } _response_assertions( @@ -149,7 +148,6 @@ def test_index(version, product): "Content-Length": str(len(expected_result)), "Content-Type": "application/json", "Version": version, - "Location": None, }, } _response_assertions( From acae57dbc2f9ffc00ffb4114b27200e645bdffbe Mon Sep 17 00:00:00 2001 From: jameslinnell Date: Thu, 7 Nov 2024 12:24:19 +0000 Subject: [PATCH 14/20] Add delete to smoke tests --- src/api/tests/smoke_tests/test_smoke.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/api/tests/smoke_tests/test_smoke.py b/src/api/tests/smoke_tests/test_smoke.py index 68d86720e..5ebf78559 100644 --- a/src/api/tests/smoke_tests/test_smoke.py +++ b/src/api/tests/smoke_tests/test_smoke.py @@ -14,6 +14,8 @@ def _request(base_url: str, headers: dict, path: str, method: str): if method == "POST": body = json.dumps({"foo": "bar"}) return requests.post(url=url, headers=headers, data=body) + elif method == "DELETE": + return requests.delete(url=url, headers=headers) return requests.get(url=url, headers=headers) From 8962c09564474e1564e53d8b3adcfd96a6ccd1a0 Mon Sep 17 00:00:00 2001 From: jameslinnell Date: Thu, 7 Nov 2024 15:30:02 +0000 Subject: [PATCH 15/20] Workflow test --- .github/workflows/merge.yml | 5 ++++ .github/workflows/on_pr_close.yml | 45 +++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) create mode 100644 .github/workflows/on_pr_close.yml diff --git a/.github/workflows/merge.yml b/.github/workflows/merge.yml index 1673d54ac..b97d4c64e 100644 --- a/.github/workflows/merge.yml +++ b/.github/workflows/merge.yml @@ -10,6 +10,11 @@ permissions: contents: read actions: write +env: + BRANCH_NAME: ${{ github.event.pull_request.head.ref }} + BRANCH_GITHUB_SHA_SHORT: $(echo ${{ github.event.pull_request.head.sha }} | cut -c 1-7) + TF_CLI_ARGS: -no-color + jobs: get-branch-from-workflow-file: runs-on: [self-hosted, ci] diff --git a/.github/workflows/on_pr_close.yml b/.github/workflows/on_pr_close.yml new file mode 100644 index 000000000..54de669ca --- /dev/null +++ b/.github/workflows/on_pr_close.yml @@ -0,0 +1,45 @@ +name: "On PR Close" + +permissions: + id-token: write + contents: read + actions: write + +env: + TF_CLI_ARGS: -no-color + CI_ROLE_NAME: ${{ secrets.CI_ROLE_NAME }} + BRANCH_NAME: ${{ github.event.pull_request.head.ref }} + BRANCH_GITHUB_SHA_SHORT: $(echo ${{ github.event.pull_request.head.sha }} | cut -c 1-7) + +on: + pull_request: + types: [closed] + +jobs: + parse-secrets: + runs-on: [self-hosted, ci] + steps: + - id: parse-secrets + run: | + echo "::add-mask::${{ secrets.CI_ROLE_NAME }}" + + build-base: + runs-on: [self-hosted, ci] + needs: [parse-secrets] + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ env.BRANCH_NAME }} + - uses: ./.github/actions/make/ + with: + command: build + save-to-cache: "true" + restore-from-cache: "false" + + destroy-test: + runs-on: [self-hosted, ci] + needs: [parse-secrets, build-base] + steps: + - name: Branch to destroy + run: | + echo "This will destroy all workspaces associated with branch name: ${{ github.head_ref }}" From c935b217dcf59c153cbef12e43a5e8c5147130e0 Mon Sep 17 00:00:00 2001 From: jameslinnell Date: Thu, 7 Nov 2024 16:25:38 +0000 Subject: [PATCH 16/20] Destroy workflow on close PR --- .github/workflows/merge.yml | 5 ----- .github/workflows/on_pr_close.yml | 21 +++++++++++++-------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/.github/workflows/merge.yml b/.github/workflows/merge.yml index b97d4c64e..1673d54ac 100644 --- a/.github/workflows/merge.yml +++ b/.github/workflows/merge.yml @@ -10,11 +10,6 @@ permissions: contents: read actions: write -env: - BRANCH_NAME: ${{ github.event.pull_request.head.ref }} - BRANCH_GITHUB_SHA_SHORT: $(echo ${{ github.event.pull_request.head.sha }} | cut -c 1-7) - TF_CLI_ARGS: -no-color - jobs: get-branch-from-workflow-file: runs-on: [self-hosted, ci] diff --git a/.github/workflows/on_pr_close.yml b/.github/workflows/on_pr_close.yml index 54de669ca..9a0be3862 100644 --- a/.github/workflows/on_pr_close.yml +++ b/.github/workflows/on_pr_close.yml @@ -9,7 +9,6 @@ env: TF_CLI_ARGS: -no-color CI_ROLE_NAME: ${{ secrets.CI_ROLE_NAME }} BRANCH_NAME: ${{ github.event.pull_request.head.ref }} - BRANCH_GITHUB_SHA_SHORT: $(echo ${{ github.event.pull_request.head.sha }} | cut -c 1-7) on: pull_request: @@ -36,10 +35,16 @@ jobs: save-to-cache: "true" restore-from-cache: "false" - destroy-test: - runs-on: [self-hosted, ci] - needs: [parse-secrets, build-base] - steps: - - name: Branch to destroy - run: | - echo "This will destroy all workspaces associated with branch name: ${{ github.head_ref }}" + destroy-pr-workspaces: + runs-on: [self-hosted, ci] + needs: [parse-secrets, build-base] + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ env.BRANCH_NAME }} + fetch-depth: 0 + - name: Remove PR workspaces + uses: ./.github/actions/make/ + with: + command: destroy--redundant-workspaces BRANCH_NAME=origin/${{ env.BRANCH_NAME }} DESTROY_ALL_COMMITS_ON_BRANCH=true KILL_ALL=true TF_CLI_ARGS=${{ env.TF_CLI_ARGS }} + requires-aws: true From 59a73bf076257f402047ead31f7b5e6313082e8f Mon Sep 17 00:00:00 2001 From: jameslinnell Date: Thu, 7 Nov 2024 17:04:04 +0000 Subject: [PATCH 17/20] Use dev env --- scripts/infrastructure/destroy/destroy-redundant-workspaces.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/scripts/infrastructure/destroy/destroy-redundant-workspaces.sh b/scripts/infrastructure/destroy/destroy-redundant-workspaces.sh index 060aa5d10..62b4db6d5 100644 --- a/scripts/infrastructure/destroy/destroy-redundant-workspaces.sh +++ b/scripts/infrastructure/destroy/destroy-redundant-workspaces.sh @@ -37,12 +37,11 @@ function _destroy_redundant_workspaces() { workspaces=$(aws s3 ls "$bucket" --no-paginate | awk '{print $NF}' | sed 's:/$::') # get JIRA ID from branch name + ENVIRONMENT="dev" if [[ $BRANCH_NAME =~ feature\/(PI-[0-9]+)[-_] ]]; then workspace_id="${BASH_REMATCH[1]}" - ENVIRONMENT="ref" elif [[ $BRANCH_NAME == *release/* ]]; then workspace_id="${BRANCH_NAME##*release/}" - ENVIRONMENT="dev" fi echo "The workspace ID is: $workspace_id" echo "Destroying workspaces in: $ENVIRONMENT" From a5a81e79d7bddb09b1cee1d5c81fd93299175063 Mon Sep 17 00:00:00 2001 From: jameslinnell Date: Thu, 7 Nov 2024 17:56:24 +0000 Subject: [PATCH 18/20] Destroy workflow on close PR --- .github/workflows/on_pr_close.yml | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/.github/workflows/on_pr_close.yml b/.github/workflows/on_pr_close.yml index 9a0be3862..ed29b550c 100644 --- a/.github/workflows/on_pr_close.yml +++ b/.github/workflows/on_pr_close.yml @@ -35,16 +35,16 @@ jobs: save-to-cache: "true" restore-from-cache: "false" - destroy-pr-workspaces: - runs-on: [self-hosted, ci] - needs: [parse-secrets, build-base] - steps: - - uses: actions/checkout@v4 - with: - ref: ${{ env.BRANCH_NAME }} - fetch-depth: 0 - - name: Remove PR workspaces - uses: ./.github/actions/make/ - with: - command: destroy--redundant-workspaces BRANCH_NAME=origin/${{ env.BRANCH_NAME }} DESTROY_ALL_COMMITS_ON_BRANCH=true KILL_ALL=true TF_CLI_ARGS=${{ env.TF_CLI_ARGS }} - requires-aws: true + destroy-pr-workspaces: + runs-on: [self-hosted, ci] + needs: [parse-secrets, build-base] + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ env.BRANCH_NAME }} + fetch-depth: 0 + - name: Remove PR workspaces + uses: ./.github/actions/make/ + with: + command: destroy--redundant-workspaces BRANCH_NAME=origin/${{ env.BRANCH_NAME }} DESTROY_ALL_COMMITS_ON_BRANCH=true KILL_ALL=true TF_CLI_ARGS=${{ env.TF_CLI_ARGS }} + requires-aws: true From 50e6a6c714effaaf17c2641be229e4d2ca192114 Mon Sep 17 00:00:00 2001 From: jameslinnell Date: Thu, 7 Nov 2024 19:46:09 +0000 Subject: [PATCH 19/20] Destroy workflow on close PR --- .github/workflows/{on_pr_close.yml => on-pr-close.yml} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename .github/workflows/{on_pr_close.yml => on-pr-close.yml} (97%) diff --git a/.github/workflows/on_pr_close.yml b/.github/workflows/on-pr-close.yml similarity index 97% rename from .github/workflows/on_pr_close.yml rename to .github/workflows/on-pr-close.yml index ed29b550c..bcbfad05d 100644 --- a/.github/workflows/on_pr_close.yml +++ b/.github/workflows/on-pr-close.yml @@ -1,4 +1,4 @@ -name: "On PR Close" +name: "Workflow: PR Close" permissions: id-token: write From d658f03cef25653565536b708d39f115f5916131 Mon Sep 17 00:00:00 2001 From: jameslinnell Date: Fri, 8 Nov 2024 09:40:43 +0000 Subject: [PATCH 20/20] Add Destroy workflow to changelog --- CHANGELOG.md | 1 + changelog/2024-11-06.md | 1 + 2 files changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index be2ba2b04..149abb2ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - [PI-558] Add search wrapper - [PI-591] Better postman - [PI-293] E2E tests with apigee +- [PI-601] Destroy workspaces on PR close ## 2024-11-05 - [PI-585] PR/release CI builds in dev diff --git a/changelog/2024-11-06.md b/changelog/2024-11-06.md index bfe055c9f..a14fedc2d 100644 --- a/changelog/2024-11-06.md +++ b/changelog/2024-11-06.md @@ -4,3 +4,4 @@ - [PI-558] Add search wrapper - [PI-591] Better postman - [PI-293] E2E tests with apigee +- [PI-601] Destroy workspaces on PR close