Request response parameters

[sudeepkunhis]

What is the context of this PR?

https://jira.ons.gov.uk/browse/CIMS-37

This PR is to define the request and response parameters for the API endpoint

Request body -

• CIR schema JSON

Request query parameters -

• current version - string - this will be later restricted to v9.0.0 (aligned to the current Live validator version)
• target version - string - this will be later restricted to v10.0.0

Response -

• Converted CIR schema JSON (for now, this will be same as the input schema without any transformation)
• status - 200 (successful)

How to review

Run the commands make docker-build and make docker-run to spin up the local application, launch the FastAPI webpage and test the endpoint with the below sample JSON in the POST request.

{"valid_json": "valid_json"}

[liamtoozer]

Just left some comments for discussion at this early stage 👍

[berroar]

Might be better to call the src folder eq_cir_converter_service more meaningful that way, and aligns with what we do in eQ.

[sudeepkunhis]

Changed

[berroar]

Again minor but the docstring should probably include the args/return info, worth keeping these up to date from the beginning

[sudeepkunhis]

Good point, added the necessary comments

[liamtoozer]

As the tests folder has moved into the src folder in a later commit in this PR, I think this means we are copying it into the Docker image as well

[liamtoozer]

Also, not related to this PR but we should probably try to use a slim base image for python (similar to Runner). I will raise a tech debt ticket 👍

[sudeepkunhis]

Good point, used the slim Python image instead, copied only the app folder, so the tests folder is not copied into the docker image.

[liamtoozer]

We might want to keep E501 in to prevent duplicates?

[sudeepkunhis]

Added the ignore warnings

[berroar]

I think there might be an issue with the linting step at the moment, when I run locally it seems to catch some errors that are not being reported as part of the run in the actions workflow

[sudeepkunhis]

@berroar Linting is alright in local and github, all issues are solved, could you please check.

[liamtoozer]

I think to match the similar way Runner have their Dockerfile, we would keep the repo folder named as app and just name the working folder to eq_cir_converter_service at build time so like:

...
WORKDIR /eq_cir_converter_service

COPY pyproject.toml poetry.lock /eq_cir_converter_service/

RUN install the deps

COPY ./app /eq_cir_converter_service/app
...

This way, we can also handle the poetry installation steps separately, before copying the app folder later in the build

[liamtoozer]

Another point to note is that we have a docker-compose.yml file which mounts the entire app folder, which we would need to remove otherwise we would be having 2 separate loads of the app folder

[sudeepkunhis]

@liamtoozer Again, what we have now is ideal - we are installing the dependencies and then copying the eq_cir_converter_service, just renamed the WORKDIR to /root so we don't have the app folder name in two places. Also, the docker-compose is used only locally to spin up the application.

[liamtoozer]

I think the docker-compose might still be adding the repo contents and naming it as app in the volume mount config, so application code is being copied in 2 places - in root and in app?

[sudeepkunhis]

@liamtoozer As discussed, removed docker-compose.yml completely to avoid further confusion since we have only one docker container, updated the make file commands accordingly.

[liamtoozer]

Just wondering if we can try alpine instead of slim here now, as SDS are using alpine for their base image 🤔

Happy to park and revisit later, but given that we dropped image size in this PR, it might be worthwhile? Might also be worth calling this out in the PR description, or ideally a separate PR, but appreciate that it's still early days for this.

[sudeepkunhis]

@liamtoozer We already reduced the image size to around 200 MB from more than a GB initially, not sure what we are trying to achieve here.

[liamtoozer]

I raised a ticket to look into reducing the Docker image size originally in my earlier review #19 (comment), but it was then changed in this PR shortly after, so thought it could maybe be reduced even further as we are doing it in this PR anyway? Ideally, images should be as small as possible for quicker deployments, spinning up locally etc but happy to leave as slim 👍

[berroar]

In addition to the above, wherever possible we should be trying to make sure any images we are using only contain the minimum/only what they need to run as this minimises the number of potential security vulnerabilities, so it not just the speed/size considerations (which are also important).

[liamtoozer]

V minor - We should try and use os.getenv() where possible

Also, appreciate this is just for logging, but I think the function might need to return int and handle incorrect env var logging level if getattr doesn't find a logger match for an incorrectly named LOG_LEVEL? We can default with something like getattr(logging, log_level, logging.INFO) or something similar?

[sudeepkunhis]

@liamtoozer This is already taken care in the line log_level = os.environ.get("LOG_LEVEL", "INFO"), it returns INFO by default when LOG_LEVEL is not defined. The Any return type is correct since it aligns with the return type of getattr

[berroar]

We need to document log_level (and any other env vars) in the readme, as if it is not set to a valid value the app will fall over. I think the type hinting does need to be updated as in reality only an int can ever be returned here without the app breaking. Also since this can break the app if set incorrectly would be good to have a test for this, when parsing is added/this is handled.

[sudeepkunhis]

@berroar Okay, there was a lint error when the return type was changed to int, it is sorted now, refactored getattr with the default setting of INFO. Documented LOG_LEVEL setting in README, also added unit tests for the logging level configuration.