-
Notifications
You must be signed in to change notification settings - Fork 163
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
Use docker.sh on the CI #2010
Use docker.sh on the CI #2010
Conversation
e3bcda6
to
04e4e0f
Compare
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.
Reviewed 10 of 11 files at r1.
Reviewable status: 10 of 11 files reviewed, 5 unresolved discussions (waiting on @worxli)
docker.sh, line 123 at r1 (raw file):
docker container create $args scion -c "tail -f /dev/null" docker start "$cntr" docker exec scion /docker-entrypoint.sh
Add a comment explaining why this is here.
docker.sh, line 193 at r1 (raw file):
$PROGRAM base $PROGRAM build $PROGRAM run
Needs updating - missing start
, exec
, stop
.
.circleci/config.yml, line 43 at r1 (raw file):
command: | set +e -x mkdir -p "/tmp/artifacts"
The new approach loses the top-level directory in the artifacts tarball. I.e. if i unpack the new artifact, i get a dir named "artifacts", instead of something meaningful. Proposal:
mkdir -p /tmp/artifacts/mount
SCION_MOUNT=/tmp/artifacts/mount ./docker.sh start
Then in clean up:
mkdir -p "/tmp/artifacts.out"
mv /tmp/artifacts/{mount,"$ARTIFACTS"}
tar caf "/tmp/artifacts.out/$ARTIFACTS.tar.gz" -C /tmp/artifacts "$ARTIFACTS"
tools/ci/local, line 18 at r1 (raw file):
[ -n "$DOCKER" ] && { make -sC docker/perapp || exit 1; } docker container inspect "$cntr" &>/dev/null && { echo "Removing stale container"; docker rm -f "$cntr"; }
I think this is redundant - docker.sh start
will take care of it.
tools/ci/local, line 20 at r1 (raw file):
docker container inspect "$cntr" &>/dev/null && { echo "Removing stale container"; docker rm -f "$cntr"; } tmpdir=$(mktemp -d /tmp/artifacts.XXXXXXX)
Output this dir here, too.
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.
Reviewable status: 10 of 11 files reviewed, 6 unresolved discussions (waiting on @worxli)
docker.sh, line 132 at r1 (raw file):
cmd_stop() { local cntr="scion" echo "Stopping $cntr container"; docker stop "$cntr";
docker rm -f "$cntr"
?
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.
Reviewed 1 of 11 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @worxli)
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.
Reviewable status: 8 of 11 files reviewed, 6 unresolved discussions (waiting on @kormat and @worxli)
docker.sh, line 123 at r1 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Add a comment explaining why this is here.
Done.
docker.sh, line 132 at r1 (raw file):
Previously, kormat (Stephen Shirley) wrote…
docker rm -f "$cntr"
?
Done.
docker.sh, line 193 at r1 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Needs updating - missing
start
,exec
,stop
.
Done.
.circleci/config.yml, line 43 at r1 (raw file):
Previously, kormat (Stephen Shirley) wrote…
The new approach loses the top-level directory in the artifacts tarball. I.e. if i unpack the new artifact, i get a dir named "artifacts", instead of something meaningful. Proposal:
mkdir -p /tmp/artifacts/mount SCION_MOUNT=/tmp/artifacts/mount ./docker.sh start
Then in clean up:
mkdir -p "/tmp/artifacts.out" mv /tmp/artifacts/{mount,"$ARTIFACTS"} tar caf "/tmp/artifacts.out/$ARTIFACTS.tar.gz" -C /tmp/artifacts "$ARTIFACTS"
Done.
tools/ci/local, line 18 at r1 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I think this is redundant -
docker.sh start
will take care of it.
Done.
tools/ci/local, line 20 at r1 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Output this dir here, too.
Not needed.
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.
Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kormat)
docker.sh, line 132 at r1 (raw file):
Previously, worxli (Lukas Bischofberger) wrote…
Done.
-f
means it will stop the container if it's running, so there's no need for two docker
commands.
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kormat)
docker.sh, line 132 at r1 (raw file):
Previously, kormat (Stephen Shirley) wrote…
-f
means it will stop the container if it's running, so there's no need for twodocker
commands.
Ah thought you wanted to have it nicely done.
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kormat and @worxli)
tools/ci/integration, line 7 at r2 (raw file):
./docker.sh exec "set -x; ./scion.sh topology ${DOCKER:+-d}"
Change this to ${DOCKER:+zkclean -d}
until we can isolate the host zk properly, but add a FIXME, as this breaks the host env.
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kormat and @worxli)
docker.sh, line 132 at r1 (raw file):
Previously, worxli (Lukas Bischofberger) wrote…
Ah thought you wanted to have it nicely done.
Oh. I see your point. I didn't realise it uses kill -9. Ignore the comment then, and drop -f
:)
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kormat and @worxli)
docker.sh, line 132 at r1 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Oh. I see your point. I didn't realise it uses kill -9. Ignore the comment then, and drop
-f
:)
Ah, but let's use tools/quiet
so we don't get noise.
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @worxli and @kormat)
tools/ci/integration, line 7 at r2 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Change this to
${DOCKER:+zkclean -d}
until we can isolate the host zk properly, but add a FIXME, as this breaks the host env.
Done.
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.
Reviewed 1 of 2 files at r3.
Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion (waiting on @kormat and @worxli)
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.
Reviewed 1 of 1 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved
a996329
to
7600686
Compare
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.
Reviewed 1 of 2 files at r5.
Reviewable status: 11 of 12 files reviewed, all discussions resolved
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.
Reviewed 1 of 2 files at r5.
Reviewable status:complete! all files reviewed, all discussions resolved
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)