Add production Dockerfile and ci image upload workflow #70

asuresh-code · 2024-11-25T14:07:18Z

Description

Enter a description of the changes here

Testing instructions

Add a set up instructions describing how the reviewer should test the code

Review code
Check Actions build
Review changes to test coverage
{more steps here}

Agile board tracking

closes #5

codecov-commenter · 2024-11-25T14:11:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.57%. Comparing base (d51376a) to head (11ae8d0).
Report is 40 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #70      +/-   ##
===========================================
+ Coverage    99.28%   99.57%   +0.28%     
===========================================
  Files           19       19              
  Lines          422      468      +46     
===========================================
+ Hits           419      466      +47     
+ Misses           3        2       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

VKTB

Still waiting on the Harbor project. The README will need to be updated at the end to include instructions.

.github/workflows/.ci.yml

Dockerfile.prod

VKTB · 2024-12-12T12:19:42Z

Dockerfile.prod

+RUN set -eux; \
+    \
+    # Create loging.ini from its .example file \
+    cp object_storage_api/logging.example.ini object_storage_api/logging.ini;


We don't do this in other projects. We have instructions to tell the user to create the file manually before building the image. I think we should stay consistent.

Ah, I saw we did this in the IMS api repo, so I assumed we were following that model.

My bad, we do this in other projects, you are right. However, I think this is unnecessary because the instructions tell the user to create the file manually before building the image. The new SciGateway Auth Dockerfile does not copy this in and at some point, I am going to refactor IMS API and LDAP JWT AUth to be consistent with SciGateway Auth and have only one Dockerfile that do not copy this in and have stages for dev and prod. I suggested in my other comment that you could do the same for this repo.

Dockerfile.prod

VKTB · 2024-12-12T12:24:16Z

Dockerfile.prod

You might want to look at the Dockerfile in ral-facilities/scigateway-auth#134 and what I did there. If you like what I did then it would be nice to keep things consistent (thoughts on it in that PR are welcome). It just means that we can have a single file with multiple stages/targets.

I had a look at the PR, and I like the idea of keeping of merging the different dockerfiles into 1 file with multiple stages. My only suggestion, is if it was implemented here for us to make a script to run everything succintly.

Looking at the README of that PR, the commands to build the image, and run tests/start the container are very long, so it would be useful to have shortened commands like we do on IMS.

My only suggestion, is if it was implemented here for us to make a script to run everything succintly.

Looking at the README of that PR, the commands to build the image, and run tests/start the container are very long, so it would be useful to have shortened commands like we do on IMS.

Sorry, I am not sure I fully understand what you mean. Could you please explain with examples if possible?

It's a small point, that the command for running the containers seem like they are very long (compared to just using docker compose up), and I'm not sure if you need to build an image each time you: 1. switch from testing to developing or 2. make changes? I think you could use a docker-compose.yml to run them (shortening the commands), although I'm not sure if/how you would configure it to work with different targets for the same dockerfile (i.e docker compose test up, docker compose dev up, etc)

Thanks for your explanation, I see what you mean now. This is why in the README, the Using docker-compose.yml for local development section is at the top of the Inside of Docker section, telling the reader that this is the easiest way to run the app for local development.

As for testing, given that this app requires other services like MongoDB, minio etc to be spun up for the e2e to run, it makes sense to have a second file called docker-compose.test.yml file like you suggested. In that file you can define the services that are needed for testing and set the object-storage-api container to use the test target from the Dockerfile. You can then use the docker compose up and docker compose down to run the tests using that new file. At the same time, you should rename the current docker-compose.yml file to docker-compose.dev.yml and only use that for local development. As both Docker Compose files have the source code mounted through volumes, you will not need to rebuild an image when you make changes to the code.

SciGateway Auth doesn't require any services to be spun up so it is easier (at least for me) to just run the tests with the docker run command as I can just copy and paste it.

Please let me know if was not clear enough with my suggestion above or you need other help.

Thanks for the response, it helped clarify a lot of points. I've made all of the changes covered here now, but I had a few clarifying questions to ask:

Does prod also need its own compose file? I assume it doesn't, and that you only need the stage to push to Harbor.

In the dockerfile test stage, I'm not sure if the CMD line should run fastapi or pytest. It's currently running pytest (as I can see you did the same in the FastAPI PR), but I don't really understand why this decision is made? If I use the docker-compose.test.yml file the tests don't automatically run?

Also if both compose files are meant to be that similar (only difference being dockerfile stage target), I think we could configure it to accept an environment variable specificying the stage and just keep 1 compose file

Does prod also need its own compose file? I assume it doesn't, and that you only need the stage to push to Harbor.

Not really because we are not spinning up our own instances of S3 and MongoDB in production. Even if we did not have these additional services (like is the case with SciGateway Auth), having a prod compose file will not fit all cases because the prod setup can vary i.e. one can decide to use a different reverse proxy and deploy some sort of metric services.

This highlights a case where a developer can have S3 and MongoDB set up locally on their machine outside of Docker so the instructions (in SciGateway Auth) on running the tests with docker run (without using the docker compose file) would be useful here.

In the dockerfile test stage, I'm not sure if the CMD line should run fastapi or pytest. It's currently running pytest (as I can see you did the same in the FastAPI PR), but I don't really understand why this decision is made? If I use the docker-compose.test.yml file the tests don't automatically run?

That's because the test stage is meant for running the tests (unit, e2e etc) locally as opposed to running the application so when you start a container with that image, it would run the tests. The reason we decided to do this is that we always run the tests in the same environment. The dev stage should be used for running the app locally whereas the prod stage for running in prod.

Regarding the tests not running, I will have a look next week because they should do if everything is configured correctly.

Also if both compose files are meant to be that similar (only difference being dockerfile stage target), I think we could configure it to accept an environment variable specificying the stage and just keep 1 compose file

While I am a fan of minimising duplicated code/configurations, I think this could get messy if the number of differences grows in future so I am not sure. I am also not sure if you can do something like that with ENV VARs but we can look into it.

In the latest commit I have an example implementation of using Environment Variables for the compose file.

I do agree if there would be more differences in the future, then keeping them separate sounds like a good idea since both commands are of similar length, and there wouldn't likely be more compose files in the future.

Regarding the tests not running, I will have a look next week because they should do if everything is configured correctly.

I spent too long on this today.. I discovered that Docker Compose doesn't respect the target if there is already a Docker image for the container. For example, if I run docker compose -f docker-compose.dev.yml up, it will build an image for the object-storage-api container and spin up a container that will start the app. If I then stop the containers (using Ctrl+c) and run TARGET_STAGE=test docker compose -f docker-compose.dev.yml up, it will not build a new image for the the object-storage-api but use the one it built before which is why it doesn't run the tests. Rebuilding the images (docker compose -f docker-compose.dev.yml up) before running the Docker Compose up command each time the target is switched seems to solve this problem.

However, this issue does not arise (and no rebuilding of images is required) if the two compose files are used and different service and container names are used in each of the files. For example, naming the service object-storage-api-test and the container object_storage_api_test_container in the docker-compose.test.yml.

Ah I see, well in that case it seems even more appropriate to keep separate compose files in this case.

…hanging to 1 dockerfile for repo #5

…kerfile-and-CI-image-upload-workflow-#5

VKTB · 2025-01-16T15:46:46Z

.github/workflows/.ci.yml

+    # This job triggers only if all the other jobs succeed. It builds the Docker image and if successful,
+    # it pushes it to Harbor.


Comment needs updating as it won't push it every time anymore.

VKTB · 2025-01-16T15:48:22Z

docker-compose.yml

This is only used for local dev, not prod. Please revert the changes so it uses the Dockerfile.

VKTB · 2025-01-28T08:01:12Z

Dockerfile

@@ -1,4 +1,4 @@
-FROM python:3.12.8-alpine3.20@sha256:0c4f778362f30cc50ff734a3e9e7f3b2ae876d8386f470e0c3ee1ab299cec21b
+FROM python:3.12.8-alpine3.20@sha256:0c4f778362f30cc50ff734a3e9e7f3b2ae876d8386f470e0c3ee1ab299cec21b as base


You don't really need a base stage in this case. You can just move this to the dev stage considering that the tests stage uses it as the base.

VKTB · 2025-01-28T08:02:01Z

Dockerfile

+
+COPY test/ test/
+
+CMD ["pytest", "--config-file", "test/pytest.ini", "test/", "--cov"]


This would fail because only the prod dependencies are installed.

I've opted to install from the pyproject.toml instead to get the test dependencies, which I believe should solve the issue.

VKTB · 2025-01-28T08:14:29Z

.github/workflows/.ci.yml

          sleep 10
      - name: Create MinIO buckets
        run: |
-          docker compose up minio_create_buckets
+          TARGET_STAGE=test docker compose -f docker-compose.dev.yml up minio_create_buckets

      - name: Run e2e tests
        run: pytest -c test/pytest.ini test/e2e/ --cov


Tests should now run in a Docker container. This requires some thinking given there are multiple jobs for the tests.

1 solution I've thought of is in the Dockerfile, we can split up the build stage for test into a unit test stage, and e2e test stage (the only difference being the command run).

Then in the workflow in the run e2e/unit test step of the separate jobs, we build the image targeting the unit or e2e stage respectively, and then run the image instead of using the docker compose file.

docker build --target unit-test -t object-storage-api:unit-test . docker run object-storage-api:unit-test

This saves us having to create a new compose file for unit/e2e testing, or introduce environment variables into the compose file. This does create a new problem of now the docker-compose.test.yml file no longer has a target stage that runs all of the tests. This could be addressed by:

Define a 3rd test stage in the Dockerfile that runs all tests (same as existing test stage essentially), and set that as the target for the compose file.

Create 2 separate services + containers in the compose file 1 for unit testing and 1 for e2e testing. The separate jobs can now just run each service to their respective tests, and to run all tests you can just run everything? I'm less sure about this approach, I don't know if the services would conflict with each other if you try to run them at the same time. I think the only other thing that would need to be changed is there port numbers, so that they're not the same?

services: object-storage-api-unit-test: container_name: object_storage_api_container_unit_test build: context: . target: unit-test ... object-storage-api-e2e-test: container_name: object_storage_api_container_e2e_test build: context: . target: e2e-test ...

docker-compose -f docker-compose.test.yml up object-storage-api-unit-tests

VKTB · 2025-01-30T10:16:44Z

@asuresh-code Thanks for all your work on this PR so far, as agreed I will continue working on it.

asuresh-code added 2 commits November 22, 2024 13:33

added production dockerfile, started multi-stage build #5

4bba5fe

add production dockerfile and multi-stage build for installations #5

cd4ca0e

asuresh-code self-assigned this Nov 25, 2024

asuresh-code marked this pull request as draft November 25, 2024 14:07

asuresh-code added the docker Pull requests that update Docker code label Nov 25, 2024

asuresh-code added 2 commits November 25, 2024 16:32

change usergroup to nobody #5

ba8bfc3

add CI image upload workflow #5

05a7082

VKTB reviewed Dec 12, 2024

View reviewed changes

asuresh-code added 2 commits January 15, 2025 14:19

add tag github workflow, and simplified prod file TODO: Investigate c…

4e06f88

…hanging to 1 dockerfile for repo #5

Merge remote-tracking branch 'origin/develop' into add-production-doc…

bf5e983

…kerfile-and-CI-image-upload-workflow-#5

asuresh-code force-pushed the add-production-dockerfile-and-CI-image-upload-workflow-#5 branch from bccd108 to 29adf12 Compare January 16, 2025 08:28

matched prod python version to dev #5

a2d28e0

asuresh-code force-pushed the add-production-dockerfile-and-CI-image-upload-workflow-#5 branch from 29adf12 to a2d28e0 Compare January 16, 2025 08:36

asuresh-code marked this pull request as ready for review January 16, 2025 09:07

asuresh-code requested a review from VKTB January 16, 2025 09:09

test-harbor-push #5

1b2aa49

VKTB requested changes Jan 16, 2025

View reviewed changes

Address PR Review Comments #5

f97a991

asuresh-code requested a review from VKTB January 20, 2025 09:08

Merged dockerfiles into 1 file, and split compose files up #5

7ec2aec

asuresh-code force-pushed the add-production-dockerfile-and-CI-image-upload-workflow-#5 branch 2 times, most recently from dc3e4b3 to 5fb8ba9 Compare January 24, 2025 11:44

fix for github workflow #5

9a87e9c

asuresh-code force-pushed the add-production-dockerfile-and-CI-image-upload-workflow-#5 branch from 5fb8ba9 to 9a87e9c Compare January 24, 2025 11:52

test using ENV variable to set state #5

823c294

VKTB requested changes Jan 28, 2025

View reviewed changes

asuresh-code added 3 commits January 29, 2025 12:05

Address some PR comments TODO: Address container problem #5

11ae8d0

Possible implementation to address github workflow issue #5

2decebd

Possible implementation to address github workflow issue #5

2f74518

asuresh-code assigned VKTB Jan 31, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add production Dockerfile and ci image upload workflow #70

Add production Dockerfile and ci image upload workflow #70

asuresh-code commented Nov 25, 2024 •

edited

Loading

codecov-commenter commented Nov 25, 2024 •

edited

Loading

VKTB left a comment

VKTB Dec 12, 2024

asuresh-code Jan 16, 2025

VKTB Jan 16, 2025

VKTB Dec 12, 2024 •

edited

Loading

asuresh-code Jan 20, 2025

VKTB Jan 22, 2025 •

edited

Loading

asuresh-code Jan 23, 2025 •

edited

Loading

VKTB Jan 23, 2025 •

edited

Loading

asuresh-code Jan 24, 2025 •

edited

Loading

VKTB Jan 24, 2025 •

edited

Loading

asuresh-code Jan 27, 2025

VKTB Jan 27, 2025 •

edited

Loading

asuresh-code Jan 28, 2025

VKTB Jan 16, 2025

VKTB Jan 16, 2025

VKTB Jan 28, 2025

VKTB Jan 28, 2025

asuresh-code Jan 29, 2025

VKTB Jan 28, 2025

asuresh-code Jan 29, 2025 •

edited

Loading

VKTB commented Jan 30, 2025

		# This job triggers only if all the other jobs succeed. It builds the Docker image and if successful,
		# it pushes it to Harbor.

		@@ -1,4 +1,4 @@
		FROM python:3.12.8-alpine3.20@sha256:0c4f778362f30cc50ff734a3e9e7f3b2ae876d8386f470e0c3ee1ab299cec21b
		FROM python:3.12.8-alpine3.20@sha256:0c4f778362f30cc50ff734a3e9e7f3b2ae876d8386f470e0c3ee1ab299cec21b as base


		COPY test/ test/

		CMD ["pytest", "--config-file", "test/pytest.ini", "test/", "--cov"]

Add production Dockerfile and ci image upload workflow #70

Are you sure you want to change the base?

Add production Dockerfile and ci image upload workflow #70

Conversation

asuresh-code commented Nov 25, 2024 • edited Loading

Description

Testing instructions

Agile board tracking

codecov-commenter commented Nov 25, 2024 • edited Loading

Codecov Report

VKTB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VKTB Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VKTB Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

asuresh-code Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

VKTB Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

asuresh-code Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

VKTB Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VKTB Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asuresh-code Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

VKTB commented Jan 30, 2025

asuresh-code commented Nov 25, 2024 •

edited

Loading

codecov-commenter commented Nov 25, 2024 •

edited

Loading

VKTB Dec 12, 2024 •

edited

Loading

VKTB Jan 22, 2025 •

edited

Loading

asuresh-code Jan 23, 2025 •

edited

Loading

VKTB Jan 23, 2025 •

edited

Loading

asuresh-code Jan 24, 2025 •

edited

Loading

VKTB Jan 24, 2025 •

edited

Loading

VKTB Jan 27, 2025 •

edited

Loading

asuresh-code Jan 29, 2025 •

edited

Loading