add build push docker image image in the publish ci #311

mo-dkrz · 2024-11-14T11:47:59Z

Related Issue(s):

Description:
Since I needed to have this docker images of elastic search and open search, I added a new publish docker image step in publish.yml
ping @jonhealy1
PR Checklist:

Code is formatted and linted (run pre-commit run --all-files)
Tests pass (run make test)
Documentation has been updated to reflect changes, if applicable
Changes are added to the changelog

mo-dkrz · 2024-11-14T14:09:03Z

PR is not ready yet ... I will ping you when it's ready

mo-dkrz · 2024-11-14T16:25:32Z

it's done from my side
green pipeline:
https://github.com/mo-dkrz/stac-fastapi-elasticsearch-opensearch/actions/runs/11840542263/job/32994615762
stac-fastapi-es image:
https://github.com/mo-dkrz/stac-fastapi-elasticsearch-opensearch/pkgs/container/stac-fastapi-es
stac-fastapi-os image:
https://github.com/mo-dkrz/stac-fastapi-elasticsearch-opensearch/pkgs/container/stac-fastapi-os

ping @jonhealy1 and @jamesfisher-geo it's ready for code review. also if it;s possible please take a test on the produced images to ensure workflow works fine, thanks

jonhealy1

Really nice work here! If @jamesfisher-geo @rhysrevans3 @pedro-cf and/or @StijnCaerts have the time to take a look as well it would be really helpful. Thank you.

jamesfisher-geo · 2024-11-16T17:02:26Z

Hey @mo-dkrz great work and thanks for the contribution. I have a couple questions/comments

Could you make it clear in the readme which .env parameters are required and which are optional?

There is an issue where default values being set to empty strings if they are not defined in the .env file. For example,

If I run without WEB_CONCURRENCY set in my .env file I get:

File "/usr/local/lib/python3.11/dist-packages/uvicorn/config.py", line 365, in __init__
    self.workers = int(os.environ["WEB_CONCURRENCY"])
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: invalid literal for int() with base 10: ''

If I run without setting RELOAD in my .env file:

 File "/usr/local/lib/python3.11/dist-packages/stac_fastapi/elasticsearch/app.py", line 5, in <module>
    from stac_fastapi.api.app import StacApi
  File "/usr/local/lib/python3.11/dist-packages/stac_fastapi/api/app.py", line 31, in <module>
    from stac_fastapi.types.core import AsyncBaseCoreClient, BaseCoreClient
  File "/usr/local/lib/python3.11/dist-packages/stac_fastapi/types/core.py", line 37, in <module>
    api_settings = ApiSettings()
                   ^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/dist-packages/pydantic_settings/main.py", line 167, in __init__
    super().__init__(
  File "/usr/local/lib/python3.11/dist-packages/pydantic/main.py", line 212, in __init__
    validated_self = self.__pydantic_validator__.validate_python(data, self_instance=self)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
pydantic_core._pydantic_core.ValidationError: 1 validation error for ApiSettings
reload
  Input should be a valid boolean, unable to interpret input [type=bool_parsing, input_value='', input_type=str]
    For further information visit https://errors.pydantic.dev/2.9/v/bool_parsing

jamesfisher-geo · 2024-11-16T17:04:38Z

Generally I am curious if it is best practice to separate the backend (elasticsearch or opensearch) containers from the stac-fastapi-es and stac-fastapi-os. So the published containers are the STAC API service only @jonhealy1 @StijnCaerts @pedro-cf @rhysrevans3

jamesfisher-geo · 2024-11-16T17:05:39Z

dockerfiles/Dockerfile.ci.es

@@ -0,0 +1,75 @@
+FROM debian:bookworm-slim AS base
+
+ARG STAC_FASTAPI_TITLE


This is setting all values not specified in the .env file to empty strings. Could you modify to handle default values?

jamesfisher-geo · 2024-11-16T17:05:50Z

dockerfiles/Dockerfile.ci.os

@@ -0,0 +1,75 @@
+FROM debian:bookworm-slim AS base
+
+ARG STAC_FASTAPI_TITLE


This is setting all values not specified in the .env file to empty strings. Could you modify to handle default values?

jamesfisher-geo · 2024-11-16T17:08:16Z

README.md

+
+You need to provide a `.env` file to configure the environment variables. Here's a list of variables you can configure:
+
+- `STAC_FASTAPI_TITLE`: Title of the API shown in the documentation (default: `stac-fastapi-elasticsearch` or `stac-fastapi-opensearch`)


Could you make it clear which parameters are required?

… files since they already got defualt vals

mo-dkrz · 2024-11-16T21:01:37Z

Hey @mo-dkrz great work and thanks for the contribution. I have a couple questions/comments

Could you make it clear in the readme which .env parameters are required and which are optional?

There is an issue where default values being set to empty strings if they are not defined in the .env file. For example,

If I run without WEB_CONCURRENCY set in my .env file I get:

File "/usr/local/lib/python3.11/dist-packages/uvicorn/config.py", line 365, in __init__
    self.workers = int(os.environ["WEB_CONCURRENCY"])
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: invalid literal for int() with base 10: ''

If I run without setting RELOAD in my .env file:

 File "/usr/local/lib/python3.11/dist-packages/stac_fastapi/elasticsearch/app.py", line 5, in <module>
    from stac_fastapi.api.app import StacApi
  File "/usr/local/lib/python3.11/dist-packages/stac_fastapi/api/app.py", line 31, in <module>
    from stac_fastapi.types.core import AsyncBaseCoreClient, BaseCoreClient
  File "/usr/local/lib/python3.11/dist-packages/stac_fastapi/types/core.py", line 37, in <module>
    api_settings = ApiSettings()
                   ^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/dist-packages/pydantic_settings/main.py", line 167, in __init__
    super().__init__(
  File "/usr/local/lib/python3.11/dist-packages/pydantic/main.py", line 212, in __init__
    validated_self = self.__pydantic_validator__.validate_python(data, self_instance=self)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
pydantic_core._pydantic_core.ValidationError: 1 validation error for ApiSettings
reload
  Input should be a valid boolean, unable to interpret input [type=bool_parsing, input_value='', input_type=str]
    For further information visit https://errors.pydantic.dev/2.9/v/bool_parsing

Good point! Let’s tidy up the environment variables. It seems that STAC_FASTAPI_TITLE, STAC_FASTAPI_DESCRIPTION, and STAC_FASTAPI_VERSION already have default values. Similarly, APP_HOST, APP_PORT, and RELOAD are already defined with default values in the base model.
I don’t see any use case for ENVIRONMENT, so it can be removed. As for BACKEND, it’s only used in unit tests, so including it in Dockerfiles or the README isn’t necessary since it defaults automatically.
Plus, ES_VERIFY_CERTS and ES_USE_SSL are set to true by default, but since we want them to default to false, we can keep them as they were. Finally, I can say, none of these variables are strictly necessary anymore. I made the default values on dockerfiles and make the env section on README optional.

Done! pipeline got green, so I’ve updated the Dockerfiles and README accordingly. @jamesfisher-geo could you please take a smoke test with/without defining any env var and review again the README as well? thanks

mo-dkrz · 2024-11-16T21:20:26Z

It’s also a good idea to have all search engines in the same environment, allowing users to enable or disable a specific search engine via environment variables, such as IS_ES or IS_OS. But, it’s unlikely that users can enable both simultaneously. The main issue isn’t about running all search systems at the same time in image; it’s about how STAC FastAPI manages ports. Currently, a single STAC FastAPI instance is not configured to work with both Elasticsearch and OpenSearch in a same time. This means that if both search engines are configured and running, but one will be idle. To address this, we could exit the running image with a warning, prompting the user to select only one search engine. Or we could consider one search engine to run by default.
If you want to run two STAC FastAPI instances from one setup image, this would first require core-level development in STAC FastAPI and, secondly, would make the configuration overly complex for user. Practically speaking, the best approach would be to run one STAC FastAPI instance per search engine in an image, with the flexibility to choose the desired search system. if I'm wrong please correct me!

jonhealy1 · 2024-11-17T07:06:14Z

Generally I am curious if it is best practice to separate the backend (elasticsearch or opensearch) containers from the stac-fastapi-es and stac-fastapi-os. So the published containers are the STAC API service only @jonhealy1 @StijnCaerts @pedro-cf @rhysrevans3

@jamesfisher-geo @mo-dkrz I think James is right and the database service should be separated from the api. The stac-fastapi-elasticsearch instance for example should be able to connect to an elasticsearch instance/ cluster running anywhere. Many people will be running the api container and the database on different specialised services.

mo-dkrz · 2024-11-17T07:57:02Z

Generally I am curious if it is best practice to separate the backend (elasticsearch or opensearch) containers from the stac-fastapi-es and stac-fastapi-os. So the published containers are the STAC API service only @jonhealy1 @StijnCaerts @pedro-cf @rhysrevans3

@jamesfisher-geo @mo-dkrz I think James is right and the database service should be separated from the api. The stac-fastapi-elasticsearch instance for example should be able to connect to an elasticsearch instance/ cluster running anywhere. Many people will be running the api container and the database on different specialised services.

ähm, based on my experience, I think providing backends in a container as an optional tool offer more flexibility to users and increases its appeal due to ease of configuration and maintenance. For instance, by defining IS_ES and IS_OS, users can opt to disable both, run only the API, and rely on an external backen container hosted elsewhere by defining env vars.. Alternatively, they have the option to run both the search systems and the API within one container by activating one of IS_ES and IS_OS. This approach provides users with better control and more choices. What are your thoughts? If you agree with having optional backends in containers, please let me know to configure it. Otherwise I dismantle the backends from there. @jamesfisher-geo @jonhealy1

mo-dkrz · 2024-11-17T08:00:46Z

I think HAS_ES and HAS_OS are better naming!

jonhealy1 · 2024-11-17T09:31:07Z

@mo-dkrz having the option to turn the database in the container on or off is interesting. It would have to be well documented

jonhealy1 · 2024-11-17T09:38:51Z

Using docker compose is useful for development I think. The ghcr containers are useful for deployment but not for reflecting local changes to the api during development, unless I'm missing something here?

…ainer and updated readme

mo-dkrz · 2024-11-17T13:41:57Z

@jonhealy1 I tried to document it better in README. could you please review it again?
https://github.com/mo-dkrz/stac-fastapi-elasticsearch-opensearch/tree/add-image?tab=readme-ov-file#installation-and-running

And also docker-compose is still working as the first method ...

jamesfisher-geo · 2024-11-19T15:43:24Z

Thanks for the quick responses @mo-dkrz

I would like to suggest a change in the approach here.

For developing the API locally, it is the best-practice to clone the repo and run

docker compose up app-opensearch

or

docker compose up app-elasticsearch

Either of these will create two local Docker images, the respective backend and the STAC API, which you can use to build new features.

Published packages are generally for production use-cases. So users would pull the images of a release and deploy that to their infrastructure, pointing to their instance of elasticsearch/opensearch. It is not useful to include elasticsearch/opensearch in the same published image as the STAC API, at least not in the published images for our main branch here because:

Generally users connect the API to a managed or external instance of elasticsearch/opensearch in production.
Bundling the STAC API and backend in the same image will make updating the image and backend challenging.
Including elasticsearch/opensearch in the image increased the size of the image from ~570MB to ~3GB. That would not be appealing for users that are not using the internal instance of elasticsearch/opensearch.

The publishing steps you have are a great contribution. Would you be open to contributing the work to publish the stac-fastapi-es and stac-fastapi-os images without the backends?

mo-dkrz · 2024-11-20T19:14:45Z

Thanks @jamesfisher-geo for the review; on the weekend I will be back to dismantle the backend from image

jamesfisher-geo · 2024-11-20T21:25:02Z

Thanks @jamesfisher-geo for the review; on the weekend I will be back to dismantle the backend from image

Thanks so much!

mo-dkrz · 2024-11-24T12:12:32Z

I think it's done; I removed the backend from the images and reflected the changes on README and action as well.
Could you please review it again. @jamesfisher-geo @jonhealy1 sorry again for making the PR so long. If any changes is needed please let me know to change it. Thanks

btw I reflected the stac-utils link of images on docker-compose file. So if you want to test it, you can replace them with
ghcr.io/mo-dkrz/stac-fastapi-os:v2.2.12 for OpenSearch and ghcr.io/mo-dkrz/stac-fastapi-es:v2.2.12 for ElasticSearch

jamesfisher-geo

Sorry @mo-dkrz , a few more changes needed here. Thanks

jamesfisher-geo · 2024-11-26T13:50:34Z

README.md

+###### Key variables to configure:
+
+| Variable                     | Description                                                                          | Default                  | Required                                                                                     |
+|------------------------------|--------------------------------------------------------------------------------------|--------------------------|---------------------------------------------------------------------------------------------|


This is great

jamesfisher-geo · 2024-11-26T13:52:13Z

docker-compose.yml

@@ -3,11 +3,8 @@ version: '3.9'
 services:
  app-elasticsearch:
    container_name: stac-fastapi-es
-    image: stac-utils/stac-fastapi-es
+    image: ghcr.io/stac-utils/stac-fastapi-es:latest


Can you revert this to pulling from the local code like it was doing before (stac-utils/stac-fastapi-es)? The docker-compose file is used for local development, so we want it to build an image from the local code.

jamesfisher-geo · 2024-11-26T13:52:40Z

docker-compose.yml

    restart: always
-    build:


Revert this back to use the local Dockerfile.dev.es

jamesfisher-geo · 2024-11-26T13:52:52Z

docker-compose.yml

@@ -35,11 +32,8 @@ services:

  app-opensearch:
    container_name: stac-fastapi-os
-    image: stac-utils/stac-fastapi-os


Can you revert this to pulling from the local code like it was doing before (stac-utils/stac-fastapi-os)? The docker-compose file is used for local development, so we want it to build an image from the local code.

jamesfisher-geo · 2024-11-26T13:53:08Z

docker-compose.yml

    restart: always
-    build:


Revert this back to use the local Dockerfile.dev.os

jamesfisher-geo · 2024-11-26T13:55:05Z

dockerfiles/Dockerfile.ci.es

@@ -0,0 +1,34 @@
+FROM python:3.12-slim
+
+ENV APP_HOST="0.0.0.0"


Sorry but could you remove the ENV lines here? The environment variables should be set when you run the Docker image. Either from an ,env file or as -e tags in the docker run command. They don't need to be included in the Dockerfile.

jamesfisher-geo · 2024-11-26T13:55:10Z

dockerfiles/Dockerfile.ci.os

@@ -0,0 +1,34 @@
+FROM python:3.12-slim
+
+ENV STAC_FASTAPI_TITLE="stac-fastapi-opensearch"


Sorry but could you remove the ENV lines here? The environment variables should be set when you run the Docker image. Either from an ,env file or as -e tags in the docker run command. They don't need to be included in the Dockerfile.

mo-dkrz · 2024-11-27T08:59:59Z

@jamesfisher-geo I'v updated the change reqs, could you please review it again, thanks

jamesfisher-geo

Looks good to me. Thanks @mo-dkrz!

jonhealy1 · 2024-11-28T02:49:54Z

Thanks for the hard work here @mo-dkrz @jamesfisher-geo. The only thing I think we may be missing is some documentation explaining how to use these new docker images.

mo-dkrz · 2024-11-28T09:53:06Z

Thanks for the hard work here @mo-dkrz @jamesfisher-geo. The only thing I think we may be missing is some documentation explaining how to use these new docker images.

I made a bit of docs in README about installing and running via pre-built docker imgs, but I think it's not regular to add images docs in REAME since we have them in the pkg registry container of github and when each user needs the imgs, they pull them from there. Could you have a look and review it again @jamesfisher-geo @jonhealy1
Thanks

README.md

jonhealy1

Nice work @mo-dkrz thank you

jonhealy1 · 2024-11-30T03:21:02Z

Quite a few errors with the tests

ERROR stac_fastapi/tests/resources/test_mgmt.py::test_ping_no_param - TypeError: __init__() got an unexpected keyword argument 'app'
ERROR stac_fastapi/tests/route_dependencies/test_route_dependencies.py::test_not_authenticated - TypeError: __init__() got an unexpected keyword argument 'app'
ERROR stac_fastapi/tests/route_dependencies/test_route_dependencies.py::test_authenticated - TypeError: __init__() got an unexpected keyword argument 'app'
============ 27 passed, 6 skipped, 4 warnings, 175 errors in 16.36s ============

jonhealy1 · 2024-11-30T03:34:13Z

same errors in main branch now

jonhealy1

The tests are failing because httpx used in tests just released a new version.

mo-dkrz added 2 commits November 14, 2024 12:45

add build push docker image

6c0eac5

bump version

b19aec9

pedro-cf requested a review from jonhealy1 November 14, 2024 12:48

mo-dkrz changed the title ~~add build push docker image image in the publish ci~~ draft: add build push docker image image in the publish ci Nov 14, 2024

mo-dkrz added 2 commits November 14, 2024 16:35

add test health step in publish, refactored the dockerfiles of es and os

b24192e

furnished readme and changelog

710b8f4

mo-dkrz changed the title ~~draft: add build push docker image image in the publish ci~~ add build push docker image image in the publish ci Nov 14, 2024

jonhealy1 previously approved these changes Nov 15, 2024

View reviewed changes

jamesfisher-geo requested changes Nov 16, 2024

View reviewed changes

added default values and removed the unnecessary env vars from docker…

795fa87

… files since they already got defualt vals

mo-dkrz requested a review from jamesfisher-geo November 16, 2024 21:23

jonhealy1 self-requested a review November 17, 2024 09:36

mo-dkrz added 6 commits November 17, 2024 11:06

add WEB_CONCURRENCY in entrypoint

bd5cddf

add RUN_LOCAL_OS and RUN_LOCAL_ES option to configure backend in cont…

c97d6f0

…ainer and updated readme

better docs

7226fc1

check logs on produced images

0e70af2

added better logs, and considered all different scenarios

feb24d2

made README well documented

e8d6fc4

resolve the req change

34f36a1

mo-dkrz added 2 commits November 24, 2024 12:44

dismantled the backends from dockerfiles and chnaged the REAME

a2ee00b

prepared publish ci action

12af96a

mo-dkrz requested review from jamesfisher-geo and jonhealy1 November 24, 2024 12:12

added latest version on image links

808ee37

jamesfisher-geo requested changes Nov 26, 2024

View reviewed changes

mo-dkrz added 2 commits November 27, 2024 09:55

removed env var from dockerfiles and minor change in docker-compose

f4ab110

added forgotten build stage in docker-compose es and os services

10eba9b

mo-dkrz requested a review from jamesfisher-geo November 27, 2024 08:59

jamesfisher-geo previously approved these changes Nov 27, 2024

View reviewed changes

added docs in README

c033dfb

mo-dkrz dismissed jamesfisher-geo’s stale review via c033dfb November 28, 2024 09:49

mo-dkrz requested a review from jamesfisher-geo November 28, 2024 09:53

jonhealy1 reviewed Nov 30, 2024

View reviewed changes

README.md Show resolved Hide resolved

jonhealy1 previously approved these changes Nov 30, 2024

View reviewed changes

Merge branch 'main' into add-image

562f7b3

jonhealy1 dismissed their stale review via 562f7b3 November 30, 2024 03:16

jonhealy1 self-requested a review November 30, 2024 05:09

jonhealy1 approved these changes Nov 30, 2024

View reviewed changes

jonhealy1 merged commit 2fceed1 into stac-utils:main Nov 30, 2024
0 of 15 checks passed

		@@ -0,0 +1,75 @@
		FROM debian:bookworm-slim AS base

		ARG STAC_FASTAPI_TITLE


		You need to provide a `.env` file to configure the environment variables. Here's a list of variables you can configure:

		- `STAC_FASTAPI_TITLE`: Title of the API shown in the documentation (default: `stac-fastapi-elasticsearch` or `stac-fastapi-opensearch`)

		@@ -0,0 +1,34 @@
		FROM python:3.12-slim

		ENV STAC_FASTAPI_TITLE="stac-fastapi-opensearch"

add build push docker image image in the publish ci #311

add build push docker image image in the publish ci #311

Conversation

mo-dkrz commented Nov 14, 2024 • edited Loading

mo-dkrz commented Nov 14, 2024

mo-dkrz commented Nov 14, 2024

jonhealy1 left a comment • edited Loading

Choose a reason for hiding this comment

jamesfisher-geo commented Nov 16, 2024

jamesfisher-geo commented Nov 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mo-dkrz commented Nov 16, 2024

mo-dkrz commented Nov 16, 2024

jonhealy1 commented Nov 17, 2024

mo-dkrz commented Nov 17, 2024

mo-dkrz commented Nov 17, 2024

jonhealy1 commented Nov 17, 2024

jonhealy1 commented Nov 17, 2024

mo-dkrz commented Nov 17, 2024

jamesfisher-geo commented Nov 19, 2024

mo-dkrz commented Nov 20, 2024

jamesfisher-geo commented Nov 20, 2024

mo-dkrz commented Nov 24, 2024

jamesfisher-geo 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

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

mo-dkrz commented Nov 27, 2024

jamesfisher-geo left a comment

Choose a reason for hiding this comment

jonhealy1 commented Nov 28, 2024

mo-dkrz commented Nov 28, 2024

jonhealy1 left a comment

Choose a reason for hiding this comment

jonhealy1 commented Nov 30, 2024

jonhealy1 commented Nov 30, 2024

jonhealy1 left a comment

Choose a reason for hiding this comment

mo-dkrz commented Nov 14, 2024 •

edited

Loading

jonhealy1 left a comment •

edited

Loading

jamesfisher-geo commented Nov 16, 2024 •

edited

Loading