Add Docker Images #8671
Add Docker Images #8671
Conversation
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
I skimmed this and added some comments.
| @@ -0,0 +1,33 @@ | |||
| ARG ALPINE_IMAGE=alpine:3.22@sha256:4b7ce07002c69e8f3d704a9c5d6fd3053be500b7f1c69fc0d80990c2ad8dd412 | |||
There was a problem hiding this comment.
Why does this need to be an arg?
There was a problem hiding this comment.
So we can update it when doing security updates. I was planning on allowing the a workflow run to define it so we don't have to edit the github tag with the latest alpine tag.
| RUN addgroup -g 1000 temporal && \ | ||
| adduser -u 1000 -G temporal -D temporal && \ | ||
| mkdir -p /etc/temporal/config/dynamicconfig && \ | ||
| touch /etc/temporal/config/dynamicconfig/docker.yaml && \ |
There was a problem hiding this comment.
Does this need to be called docker.yaml?
There was a problem hiding this comment.
no, the normal config defines the path so we can change it.
|
|
||
| RUN addgroup -g 1000 temporal && \ | ||
| adduser -u 1000 -G temporal -D temporal && \ | ||
| mkdir -p /etc/temporal/config/dynamicconfig && \ |
There was a problem hiding this comment.
/etc is already for config files, why do we need the extra /config and /dynamicconfig dirs?
There was a problem hiding this comment.
I was keeping the existing pattern since the helm-charts writes to this config directory. I assume it has to exist in the image but will double check.
There was a problem hiding this comment.
I wonder how much of this can be cleaned up.
There was a problem hiding this comment.
I looked into it briefly
TEMPORAL_BROADCAST_ADDRESSis needed so the server knows what addresses to broadcast for ringpop. I don't believe0.0.0.0is sufficient- You have to specify the port for IPv4 and IPv6 differently.
- It's possible to have multiple network interfaces for so
BIND_ON_IPis needed. The interface the server is binding should be the same as what it's board casting
I'll look and see if the server itself is able to handle this in some way. I found some config that controls whether or not the server binds on localhost in the dynamic or normal config.
There was a problem hiding this comment.
We can just set good defaults for the embedded config and make sure helm has a valued. I'll make sure to include something about this in the release notes.
Removed all scripts from the server image
Co-authored-by: Roey Berman <[email protected]>
Co-authored-by: Kent Gruber <[email protected]>
|
The makefile and build scripts will be removed In a future PR. Just here for demo/testing. The Alpine Image is an argument so we can trigger builds via workflows without committing to given branch. It should make re-building images easier |
What changed?
This PR ports the Docker image building process from
temporalio/docker-buildsinto this repository. For images built from version 1.30.0 and later, the number of dependencies has been reduced.This PR focuses on the Dockerfiles and local build tooling. Future PRs will add GitHub Actions for automated builds.
Changes Made
Note: Legacy targets (legacy-server, legacy-admin-tools) are for server versions 1.26.0 -> 1.29.X (X being the highest supported version) and can be removed once support for those versions is no longer needed.
Why?
The existing build process is more complex than it has to be and existing images have dependencies that are not necessary.
How did you test it?
Potential risks
Some users might depend on some of the dependencies we've included in our images until now. Or there some special case that we haven't taken into account.