Feature/cdapi 55 #12
Feature/cdapi 55 #12
Conversation
|
Preview Lambda: |
4 similar comments
|
Preview Lambda: |
|
Preview Lambda: |
|
Preview Lambda: |
|
Preview Lambda: |
|
Preview Lambda: |
neil-sproston
left a comment
neil-sproston
left a comment
There was a problem hiding this comment.
Looks sane and reasonable.
|
Preview Lambda: |
5 similar comments
|
Preview Lambda: |
|
Preview Lambda: |
|
Preview Lambda: |
|
Preview Lambda: |
|
Preview Lambda: |
nhsd-rebecca-flynn
left a comment
nhsd-rebecca-flynn
left a comment
There was a problem hiding this comment.
After our run-through the code changes yesterday and a thorough look today, from my point of view it looks fine.
I've highlighted a few minor points, but the main one being I think we are adding too many unnecessary comments. I'd rather we named the tests well and resisted from leaving a comment explaining what the test is supposed to achieve unless it is a complicated test. None of these tests appear to be in the category and I believe we are just creating noise. Let me know your thoughts.
| Send a request to the APIs with some given parameters. | ||
| Args: | ||
| data: The data to send in the request payload | ||
| path: The path to send the request to | ||
| request_method: The HTTP method to use for the request | ||
| Returns: | ||
| Response object from the request |
There was a problem hiding this comment.
I think this could be more succinct if necessary at all.
Send a request to the APIs with data, path, request_method and returns a response object from the request
There was a problem hiding this comment.
As this client is intended to be used in multiple places, I think it's worth including a docstring to include some details around the details of this method here. I've included the docstring in this format is this is rendered by vscode in a nice standard format when displaying the docs inline.
Happy to remove these docstrings though if you do think they're not necessary even when used outside of this file?
There was a problem hiding this comment.
Ok fair enough. Let's leave as is.
|
Preview Lambda: |
1 similar comment
|
Preview Lambda: |
Also updated lambda build to package for the correct architecture.
Also updated test-unit target to include all unit tests within src directory.
… system matches expected value
Also added additional unit tests to capture missing coverage around resource.py.
…rather than via nohup
…rom correct location
… API gateway format Also updated tests to account for new _status endpoint and new create Test Result path.
nhsd-jack-wainwright
force-pushed
the
feature/CDAPI-55
branch
from
0260c3b to
39b5886
Compare
|
Preview Lambda: |
|
Preview Lambda: |
|
Preview Lambda: |
|
Preview Lambda: |
|
3 participants
Description
Updating hello world lambda to instead provide support for posting an Test Result (Bundle)
Context
This change alters the existing very simple lambda to instead accept a provided Test Result. As part of these changes the supported Test Result (Bundle) is very lightweight, but this will be built on as part of subsequent tickets.
As part of these changes, a new
api-gateway-mockcontainer is also included, allowing calls to the lambda locally in the same way that calls would be made to API gateway when running in AWS.Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.