Enable incremental builds for node{6,8,10}

[khardix]

Note: I tried to add a test for the incremental build,
but since the tests use a simulation of s2i rather than s2i itself,
I did not figure out how to do it or if it is even supported.
Improvement suggestions welcome!

[centos-ci]

Can one of the admins verify this patch?

[centos-ci]

Can one of the admins verify this patch?

[pkubatrh]

I did not figure out how to do it or if it is even supported.

No function like that currently in the framework.

I tried looking if it could be simulated the same way we do regular builds but it seems that s2i build --incremental --as-dockerfile generates a multistage Dockerfile which can only be used by podman (or at least my F28 docker does not understand the FROM ... AS syntax):

FROM test AS cached
# Save-artifacts script sourced from builder image based on user input or image metadata.
USER 1001
RUN if [ -s /usr/libexec/s2i/save-artifacts ]; then /usr/libexec/s2i/save-artifacts > /tmp/artifacts.tar; else touch /tmp/artifacts.tar; fi
FROM rhscl/nodejs-8-rhel7
LABEL "io.openshift.s2i.build.image"="rhscl/nodejs-8-rhel7" \
      "io.openshift.s2i.build.source-location"="test/test-app" \
      "io.k8s.display-name"="test:latest"

USER root
COPY --from=cached /tmp/artifacts.tar /tmp/artifacts.tar
# Copying in source code
COPY upload/src /tmp/src
# Change file ownership to the assemble user. Builder image must support chown command.
RUN chown -R 1001:0 /tmp/artifacts.tar /tmp/src
USER 1001
# Extract artifact content
RUN if [ -s /tmp/artifacts.tar ]; then mkdir -p /tmp/artifacts; tar -xf /tmp/artifacts.tar -C /tmp/artifacts; fi && \
    rm /tmp/artifacts.tar
# Assemble script sourced from builder image based on user input or image metadata.
# If this file does not exist in the image, the build will fail.
RUN /usr/libexec/s2i/assemble
# Run script sourced from builder image based on user input or image metadata.
# If this file does not exist in the image, the build will fail.
CMD /usr/libexec/s2i/run

[pkubatrh]

Maybe it could get implement as a test case in openshift tests?

[khardix]

at least my F28 docker does not understand the FROM ... AS syntax

Well, docker in Fedora is 5 (minor) versions old, so that is not much of a surprise…

If you have an idea on how the openshift test should look like, I would ask you for one; I'm really not sure how to trigger an incremental build in openshift and verify it is working as expected.

[pkubatrh]

I'm really not sure how to trigger an incremental build in openshift and verify it is working as expected.

No idea either. But will look into it.

[pkubatrh]

Is the --verbose flag supposed to be here? It generates an awful amount of output ...

[khardix]

I did add it intentionally to get a "log" that something actually happens; and when I test it locally (built with VERSION=8 TARGET=centos7), it produces one line:

$ s2i build --incremental https://github.com/sclorg/nodejs-ex s2i-nodejs8-incremental hello-node8
...snip...
---> Restoring previous build artifacts ...
'/tmp/artifacts/node_modules' -> '/opt/app-root/src/node_modules'
---> Installing application source ...
...snip...

However, if you are getting lots of output (I assume it is reporting every file it moves), it can be safely removed.

[pkubatrh]

Added a test case for the incremental build feature.

Needs sclorg/container-common-scripts#109 mergred first

[pkubatrh]

[test][test-openshift]

[pkubatrh]

Openshift failure seems like sclorg/nodejs-ex#212

[khardix]

@pkubatrh The fix for sclorg/nodejs-ex#212 might take a while to get shipped. Is there a chance for this to land before that, so that at least node8/6 might benefit from the incremental builds?

[pkubatrh]

@khardix Agreed, I do not think it is worthwhile want to wait until nodejs10 is fixed, since we do not support nodejs10 images anyway. So I am ok to get this merged even when the CI fails for that version.

Would you mind doing the review of the changes I made to the PR?

[khardix]

Would you mind doing the review of the changes I made to the PR?

The test changes look sane and, barring the Node 10 issue, seems to pass. Therefore I'm OK with merging those.

NB: I cannot hit the GH "Approve" button as an original author of this PR, so consider this my approval 😉

[pkubatrh]

Thanks for taking a look at it. Merging