using built container image #41
Conversation
DWolfsNHS
force-pushed
the
feature/GPCAPIM-166_built_image
branch
3 times, most recently
from
5fc01eb to
a1ab514
Compare
neil-sproston
left a comment
neil-sproston
left a comment
There was a problem hiding this comment.
Looks sane and appropiate
|
note: Sonar issue has previously been accepted on main |
nhsd-jack-wainwright
left a comment
nhsd-jack-wainwright
left a comment
There was a problem hiding this comment.
LGTM, just a few minor questions / comments.
|
|
||
| branch_safe = replace(replace(var.branch_name, "/", "-"), " ", "-") | ||
| log_group_name = "/ecs/preview/${local.branch_safe}" | ||
| branch_after_feature = startswith(var.branch_name, "feature-") ? substr(var.branch_name, length("feature-"), length(var.branch_name) - length("feature-")) : var.branch_name |
There was a problem hiding this comment.
Should this startswith be looking for "feature/" instead of "feature-", or perhaps be acting on the branch_safe variable instead?
There was a problem hiding this comment.
@neil-sproston - as Jack suggsest, I would expect the branch name to be feature/, but does this come after some other sanitisation of the naming that might have changed this to -?
There was a problem hiding this comment.
The variable going into terraform already has feature/ changed to feature- this is done in the action and also relates to ecr image tags etc.
| /.pyenv/bin/pyenv install ${PYTHON_VERSION} && \ | ||
| /.pyenv/bin/pyenv global ${PYTHON_VERSION} && \ | ||
| /.pyenv/bin/pyenv init - |
There was a problem hiding this comment.
Very minor but is it worth indenting these calls to indicate that they're a part of the same RUN block as above?
| echo "Installing x86 asdf executable..." ; \ | ||
| wget -O asdf.tar.gz "$ASDF_DOWNLOAD_URL/asdf-v0.18.0-linux-amd64.tar.gz"; \ | ||
| fi | ||
| fi && \ |
There was a problem hiding this comment.
As above, is it worth indenting these calls to highlight that they are being invoked as part of the same run block?
Makefile
Outdated
| @echo "Building Docker image using Docker. Utilising python version: ${PYTHON_VERSION} ..." | ||
| @$(docker) buildx build --load --provenance=false --build-arg PYTHON_VERSION=${PYTHON_VERSION} -t localhost/gateway-api-image infrastructure/images/gateway-api | ||
| @echo "Docker image 'gateway-api-image' built successfully!" | ||
| @$(docker) buildx build --load --provenance=false --build-arg PYTHON_VERSION=${PYTHON_VERSION} -t ${IMAGE_NAME} infrastructure/images/gateway-api |
There was a problem hiding this comment.
One thing that I've bumped into on the Pathology side that might be worth thinking about here is the architecture being used for the Python runtime. It might be worth explicitly declaring that we wish to build an x86 image here, as well as including the platform as part of the pip install when bundling the python dependencies.
I think the python build could be updated with something like:
@pip install ... --platform musllinux_1_1_x86_64 --only-binary=:all:
Indicating that any binaries should support musl 1.1 or higher and use the x86 architecture.
There was a problem hiding this comment.
Changes made and validated on windows with make deploy and make test as discussed
There was a problem hiding this comment.
Builds also validated in pipeline
There was a problem hiding this comment.
As well as behaviour with ephemeral AWS deployment
nhsd-jack-wainwright
left a comment
nhsd-jack-wainwright
left a comment
There was a problem hiding this comment.
Looks good to me 👍
DWolfsNHS
force-pushed
the
feature/GPCAPIM-166_built_image
branch
from
69ba897 to
49a3458
Compare
DWolfsNHS
force-pushed
the
feature/GPCAPIM-166_built_image
branch
from
49a3458 to
c0b679d
Compare
DWolfsNHS
force-pushed
the
feature/GPCAPIM-166_built_image
branch
from
c0b679d to
e6d3941
Compare
Base branch requires signed commits
|
3 participants
Description
Testing
Verified CI pipeline execution for building and deploying the image.
Ensured all tests pass successfully after addressing Sonar feedback
Context
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.