Uh oh!
There was an error while loading. Please reload this page.
ci: Fix building disabled containers - #44346
Conversation
rust-highfive
commented
Sep 5, 2017
(rust_highfive has picked a reviewer for you, use r? to override) |
arielb1
commented
Sep 5, 2017
r? @aidanhs (I think) |
Unfortunately this isn't right. A lot of these dockerfiles reference things in But scripts is not present in the disabled directory, so we need to sent the whole |
kallisti5
commented
Sep 5, 2017
That's all kind of silly. Would you support adjusting all the disabled Dockerfiles to properly have context within the disabled directory? That would make them portable between disabled and the normal path. |
aidanhs
commented
Sep 6, 2017
Sure. I think you'd just be able to generate a 'unified tar' combining both directories on the fly, unless you have something else in mind? |
kallisti5
commented
Sep 7, 2017
Couldn't the rest of the Dockerfiles in disabled omit "disabled" from the source COPY path? |
c727fe0 to
041e133Comparekallisti5
commented
Sep 7, 2017
aka, see latest changeset :-) Only two of the containers even specified disabled... the rest omit it. Now you can copy containers into the disabled directory, but still test them locally outside of travis. |
041e133 to
fa95e5aCompareHm. Some of the containers use the common scripts though which don't exist within the disabled context. I've put in a solution to this by git ignoring scripts within the disabled directory and copying the common set over during the container build, then removing them again post-build (only when building disabled containers). Not the cleanest, but I don't know of a way to add multiple contexts. |
aidanhs
commented
Sep 7, 2017
Ah so my suggestion is to generate the context by merging the directories on the fly with i.e. the transform option has moved everything upwards out of the disabled directory. You can pipe a tar to the stdin of docker build and it'll accept it as a context. |
kallisti5
commented
Sep 11, 2017
Ah. I understand now. So I started in on these changes, but docker doesn't seem too happy about it: if [ -f"$docker_dir/$image/Dockerfile" ];then
retry docker \
build \
--rm \
-t rust-ci \
-f "$docker_dir/$image/Dockerfile" \
"$docker_dir"elif [ -f"$docker_dir/disabled/$image/Dockerfile" ];thenif [ -n"$TRAVIS_OS_NAME" ];thenecho Cannot run disabled images on travis!exit 1
fi
retry tar --transform 's/^\.\/disabled\//.\//' -C $docker_dir -c .| docker \
build \
--rm \
-t rust-ci \
-f "$docker_dir/disabled/$image/Dockerfile" \
-
elseecho Invalid image: $imageexit 1
fi |
aidanhs
commented
Sep 11, 2017
Ok, the problem is that the You could fix it to work with |
* Change the context into the disabled directory. Now you can test containers which are disabled.
fa95e5a to
b04097bComparekallisti5
commented
Sep 12, 2017
Latest PR works great. Good idea on the tar transform. |
| fi | ||
| retry docker \ | ||
| # retry messes with the pipe from tar to docker. Not needed on non-travis | ||
| tar --transform 's/^\.\/disabled\//.\//' -C $docker_dir -c . | docker \ |
There was a problem hiding this comment.
The transform looks right, but it generates a very strange path for the dockerfile for me if you pipe it to tar t - .\\/Dockerfile. All of the backslashes are a bit crazy anyway, can you make # the escape character? i.e. s#^\./a/#./#'
Additionally, can you add a brief description line of what we're doing here? E.g. something like "The tar transform below makes the images in the disabled directory look like they're in the main images directory to docker build."
alexcrichton
commented
Sep 21, 2017
ping @kallisti5, just want to make sure this doesn't fall off your radar! |
alexcrichton
commented
Sep 28, 2017
Ok I'm going to close this out of inactivity, but feel free to resubmit @kallisti5! |
I'm super confused on this one. So all that needs to be changed is
2 is fine |
kallisti5
commented
Sep 28, 2017
Resubmitted in #44903 Just getting frustrated that i've refined these 6 lines so many times across so many PR's :-) |
Working on a new CI platform and noticed building disabled containers is broken. (looking at the shell script, it appears being able to build disabled containers is desired)