Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a dev target to Dockerfile, add a dev compose file #836

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gpkc
Copy link
Contributor

@gpkc gpkc commented Mar 11, 2024

About this change - What it does

  • Add a dev target to Dockerfile which will install dev and static-check dependencies
  • Set up a volume that syncs local changes into the running containers
  • Create a new compose service for barebones development and running tests

What this enables:

  • Running tests: for example docker-compose -f compose.yml -f compose-dev.yml run karapace-dev pytest -s -vvv tests/unit
  • Update containers with new code without having to rebuild everything (docker-compose down followed by docker-compose up is enough)

Why this way

See #585.

  • We add a couple new build targets: dev-builder and dev.
    • The new builder target is needed for installing the dev dependencies, since it's necessary to build wheels, and the non-slim docker base image has the necessary python-dev dependency among others.
    • The new dev target is needed for isolation. It will have the development deps installed and also changes PYTHONPATH to point to the synced folder.
  • We are syncing the sources folder from the local machine into the container via volumes in the dev compose file.
    • This way, there is no need to rebuild everything every time the source changes.
    • In conjunction with PYTHONPATH being set to this folder, every instance of e.g. python3 -m karapace ... or import karapace will be seeing the synced folder.
  • We add a new compose service: karapace-dev
    • Theoretically it would be possible to run tests using the existing services karapace-registry and karapace-rest, but that would require overriding their configurations in a manner that would mix things up too much. Instead, I've opted to create a new service that has the clear purpose of being used directly in the test-develop cycle.

In the future, one can also add a watcher to the running servers, so that not even restarting the container is necessary.

- Add a dev target to Dockerfile which will install dev and static-check dependencies
- Set up a volume that syncs local changes into the running containers
- Create a new compose service for barebones development and running tests
@gpkc gpkc requested review from a team as code owners March 11, 2024 17:09
@@ -53,3 +53,32 @@ USER karapace

HEALTHCHECK --interval=10s --timeout=30s --retries=3 --start-period=60s \
CMD python3 healthcheck.py http://localhost:$KARAPACE_PORT/_health || exit 1


FROM builder AS dev-builder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Introducing the new stages means we must update the release flow so that we don't start publishing dev images: https://github.com/Aiven-Open/karapace/blob/main/.github/workflows/container.yml.

Similarly, I think if we're to keep the two compose files, target must be specified in the existing one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple solution for this is simply to have the actual production stage come after the dev stage. This way, the default will always be the production one. And by correctly using dependencies, when building for the prod stage (implicitly or not), docker won't run any of the build code for dev.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like the best solution 👍

container/compose-dev.yml Outdated Show resolved Hide resolved
container/Dockerfile Show resolved Hide resolved
@gpkc gpkc requested a review from aiven-anton March 12, 2024 14:55
@aiven-anton
Copy link
Contributor

Funny logs from the smoke test ... 🤪

container container-karapace-dev-1 exited (0)
Error: Process completed with exit code 1.

@gpkc
Copy link
Contributor Author

gpkc commented Mar 12, 2024

Hmm why did that happen. Is the test checking if the services run indefinitely or something like that? So if they end, it fails

@aiven-anton
Copy link
Contributor

Hmm why did that happen. Is the test checking if the services run indefinitely or something like that? So if they end, it fails

No, the test just checks a single basic API call returns successfully, but the CI flow did not reach the test stage, it fails at the build stage. I think the logs just looks funny because of concurrency.

I think because we introduce the special dev service in the compose file, let's leave karapace-registry and karapace-rest intact and running the prod target? I think we should also update the smoke test to only start karapace-registry and karapace-rest.

It could perhpas also make sense to add something like this to the smoke test to give confidence test dependencies aren't installed in the default target:

if ! docker compose exec karapace-registry which pytest >/dev/null; then
    echo 'Error: test dependencies found in production image'
    exit 1
fi

@gpkc
Copy link
Contributor Author

gpkc commented Mar 13, 2024

I think because we introduce the special dev service in the compose file, let's leave karapace-registry and karapace-rest intact and running the prod target? I think we should also update the smoke test to only start karapace-registry and karapace-rest.

The main issue with that for me is the development cycle, as you will be required to rebuild the image every time you make a change on the code. I guess theoretically I could just move the PYTHONPATH setting to the compose file, I'll see if that works fine

@aiven-anton
Copy link
Contributor

aiven-anton commented Mar 13, 2024

The main issue with that for me is the development cycle, as you will be required to rebuild the image every time you make a change on the code. I guess theoretically I could just move the PYTHONPATH setting to the compose file, I'll see if that works fine

But wouldn't you always develop locally against the new dev service?

The smoke test must run the production target. If we introduce some separate means to achieve that, it's completely fine either way IMO (separate service, or separate compose file).

@gpkc
Copy link
Contributor Author

gpkc commented Mar 13, 2024

But wouldn't you always develop locally against the new dev service?

I'm not sure, perhaps you want to make changes and quickly check them in the running REST or registry services? The dev service doesn't contain any of the env vars, etc which are present in the other services

That's why I'd say it could be useful to have the -dev.yml compose, so that the normal compose.yml can be used if you want to have an environment that is closer to prod (e.g. for smoke tests), whereas the compose-dev.yml would modify the target to be dev for fast local development

@aiven-anton
Copy link
Contributor

@gpkc I think your reasoning is sound, my knee-jerk reaction against multiple compose files was just for attempting to keeping things simpler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants