dockerize signature aggregator #435
Conversation
feuGeneA
force-pushed
the
dockerize-sig-aggregator
branch
from
878c023 to
161e6df
Compare
| @@ -0,0 +1,6 @@ | |||
| FROM debian:11-slim | |||
There was a problem hiding this comment.
Compare to the old repo_root/Dockerfile. I guess I made too many changes for the PR to reflect that the file was just renamed and edited.
The edits I made were to change the base image and to drop the build ARG. The base image was formerly golang, but we're not actually building any Go code in the building of this image (rather, goreleaser is pre-building the binary, and then the Dockerfile just copies that binary into the image) so there's no need to have a Go build environment. Furthermore, there's no reason to pass in a GO_VERSION either.
feuGeneA
requested review from
iansuvak,
geoff-vball,
minghinmatthewlam,
bernard-avalabs,
michaelkaplan13 and
cam-schultz
feuGeneA
force-pushed
the
dockerize-sig-aggregator
branch
4 times, most recently
from
d52b417 to
d5ec489
Compare
feuGeneA
force-pushed
the
dockerize-sig-aggregator
branch
from
d5ec489 to
a491deb
Compare
| # The GO_VERSION must be set explicitly to be used in the Dockerfile. | ||
| - name: Set Go version | ||
| run: | | ||
| source ./scripts/versions.sh | ||
| echo GO_VERSION=$GO_VERSION >> $GITHUB_ENV |
There was a problem hiding this comment.
I don't think this step is necessary anymore now that the go.mod file can contain the patch version and actions/setup-go below accepts it
There was a problem hiding this comment.
great catch! thank you! addressed in 7c289ce, and validated in https://github.com/ava-labs/awm-relayer/actions/runs/10724002269 and https://github.com/ava-labs/awm-relayer/actions/runs/10724006401
| FROM debian:11-slim | ||
| COPY signature-aggregator /usr/bin/signature-aggregator | ||
| EXPOSE 8080 | ||
| EXPOSE 8081 |
There was a problem hiding this comment.
Why are we exposing this metrics port here but not in the relayer?
There was a problem hiding this comment.
Great question. I did it for the aggregator because it felt like the right thing to do, since there'd be no way to consume metrics without it, but I left things the way they were for the relayer in order to limit the scope of changes.
There's a similar question to be asked about why the relayer sets a USER. @cam-schultz once advised me to avoid setting a USER.
There was a problem hiding this comment.
I think we should be consistent and remove it from here since it looks like relayer handles without exposing it directly.
There was a problem hiding this comment.
after doing a little more research, it looks like EXPOSE is purely documentation, with a bit of automation convenience that takes advantage of that documentation.
when you run an image with docker run, there's a -P option that will automatically "publish" all of the ports that the image EXPOSEs, but you can manually choose to publish that port, or not, or publish different ports, and the image can also listen on other ports (eg if a config file said to use a different port), and the runtime can still publish ports that were never even EXPOSEd.
so, it seems completely unnecessary, but good documentation, and potentially a helpful convenience to some users. my vote would be to keep it, but i don't feel that strongly about it.
There was a problem hiding this comment.
Yeah it all makes sense. What this is documenting is default ports that are probably going to be overwritten by the config that you pass in. But I don't feel strongly either actually so can drop
Why this should be merged
fixes #425
How was this tested
by creating
...-testtags off of the commit here to trigger the Release Actions, the results of which can be seen here and here.