-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat: add e2e-init-chain #46
Conversation
|
||
# Copy All | ||
WORKDIR /go/src/github.com/babylonlabs-io/babylon | ||
COPY ./ /go/src/github.com/babylonlabs-io/babylon/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't it need to be:
# Version to build. Default is empty
ARG VERSION
# Copy All
WORKDIR /go/src/github.com/babylonlabs-io/babylon
COPY ./ /go/src/github.com/babylonlabs-io/babylon/
# Handle if version is set
RUN if [ -n "${VERSION}" ]; then \
git fetch origin ${VERSION}; \
git checkout -f ${VERSION}; \
fi
i.e we should be able to configure binary version here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we want this fixed version for testing, we don't need to have the version argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm correct me if I am wrong, but I have had imagined that this is needed similiary to babylond-before-upgrade
step.
i.e for babylon before upgrade we call :
docker build --tag babylonlabs-io/babylond-before-upgrade -f babylond/Dockerfile \
--build-arg VERSION="${BABYLON_VERSION_BEFORE_UPGRADE}" ${BABYLON_FULL_PATH}
to actually build the image with some old version.
I thought this will happen here too i.e
- First init chain with the
VERSION="${BABYLON_VERSION_BEFORE_UPGRADE}"
- then build image before upgrade also with
VERSION="${BABYLON_VERSION_BEFORE_UPGRADE}"
what am I missing ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will have VERSION
argument on main branch, that is to go to another version, here it will not be used to move to another version because this is already the fixed version we need to test
But if you feel more comfortable I could add it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets add it for the sake of completeness, the worst case scenario it will be unused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added version argument to dockerfile
26a3fc9
At the makefile to build the docker I did not set any argument
contrib/images/Makefile
Outdated
docker rmi babylonlabs-io/babylond-e2e-init-chain --force 2>/dev/null; true | ||
|
||
e2e-init-chain: | ||
docker build -t babylonlabs-io/babylond-e2e-init-chain --build-arg E2E_SCRIPT_NAME=chain --platform=linux/x86_64 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether --platform
shouldn't be configurable by some env variable, as when I run test locally on arm mac those fail due to native binding to bls libraries and changing platform fixes that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not know that, I will add --platform tag to docker builds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check if this 997de7c
solved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I have checked it locally and got:
> make build-docker
/Library/Developer/CommandLineTools/usr/bin/make -C contrib/images babylond
docker rmi babylonlabs-io/babylond 2>/dev/null; true
Untagged: babylonlabs-io/babylond:latest
Deleted: sha256:7cd3d4a92eff50f5490ea0636532b3fbbcea3f5585d98fb886b1e96423614339
docker build --tag babylonlabs-io/babylond -f babylond/Dockerfile /Users/konradstaniec/Work/babylon/babylon --platform=local
[+] Building 2.0s (3/3) FINISHED docker:desktop-linux
=> [internal] load build definition from Dockerfile 0.0s
=> => transferring dockerfile: 2.22kB 0.0s
=> ERROR [internal] load metadata for docker.io/library/debian:bookworm-slim 2.0s
=> ERROR [internal] load metadata for docker.io/library/golang:1.21 2.0s
------
> [internal] load metadata for docker.io/library/debian:bookworm-slim:
------
------
> [internal] load metadata for docker.io/library/golang:1.21:
------
Dockerfile:1
--------------------
1 | >>> FROM golang:1.21 AS build-env
2 |
3 | # Customize to your build env
--------------------
ERROR: failed to solve: golang:1.21: no match for platform in manifest: not found
make[1]: *** [babylond] Error 1
make: *** [build-docker] Error 2
so it seems --platform=local
does not really work on m1 macs 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thinking out loud, why do we even need this --platform=linux/x86_64
in this particular target ? Other docker building commands do not specify this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the --platform
tag at 07b4094
Add e2e-init-chain, it is necessary for e2e testing in new releases to have the init chain with the same proto versions used at the v09 bin