mprahl / s2i-angular-httpd24

S2I Builder for Angular Apps Based on the Official CentOS and RHEL httpd-24 Images
MIT License
2 stars 14 forks source link

Incremental builds fail #5

Open ddelabru opened 4 years ago

ddelabru commented 4 years ago

I've tried enabling incremental builds with the s2i builder in OpenShift 3.11, and I've hit a snag; the s2i builder can't execute the save-artifacts script due to a permissions problem:

/usr/bin/container-entrypoint: line 2: /usr/libexec/s2i/save-artifacts: Permission denied
/usr/bin/container-entrypoint: line 2: exec: /usr/libexec/s2i/save-artifacts: cannot execute: Permission denied
WARNING: Clean build will be performed because of error saving previous build artifacts

I'm not seeing similar permissions problems with any other part of the build. This is a wild guess, but I'm wondering if this could have to do with the script shebangs -- I noticed that the save-artifacts script has this shebang: https://github.com/mprahl/s2i-angular-httpd24/blob/83923ffd14fad8f45e1b1e45c21a242047a41906/s2i/bin/save-artifacts#L1 while all the other scripts have a shebang like #!/bin/bash -e instead.

mprahl commented 4 years ago

@ddelabru thanks for filing the issue. I made the following commit: https://github.com/mprahl/s2i-angular-httpd24/commit/03d0a92300e582166812d319f1751b4822af536e

I kicked off a build for this commit at: https://cloud.docker.com/repository/registry-1.docker.io/mprahl/s2i-angular-httpd24/builds/c943695e-8b02-45b7-8a34-43e909ac3999

Could you please try again?

ddelabru commented 4 years ago

@mprahl, thanks! This does appear to fix the permissions error.

Unfortunately, builds don't seem to actually cache/re-use node_modules, or at least all the dependencies are still downloaded fresh with every build and build times remain the same. There's no error, though, so I'm not immediately sure why.

If I can't figure it out in the long run I can file a separate issue for that, though.

mprahl commented 4 years ago

@ddelabru after thinking about this, the issue is that the assemble script deletes the node_modules directory at: https://github.com/mprahl/s2i-angular-httpd24/blob/master/s2i/bin/assemble#L34

This is done so that the final container images doesn't contain the node_modules directory, so it makes the container image much smaller. Taking a look at s2i-nodejs-container, they seem to compromise by deleting all the dev dependencies at: https://github.com/sclorg/s2i-nodejs-container/blob/master/8/s2i/bin/assemble#L93

How important are incremental builds for you? If they are, you or I could take that approach.

mprahl commented 4 years ago

For now, I just removed incremental build support since it was broken anyways: https://github.com/mprahl/s2i-angular-httpd24/commit/6312dcc0bf6e6a5190f8c533d8f7c5769a2bb9b4

We can continue the discussion in this issue.

ddelabru commented 4 years ago

I'll bring this up with my team. They seemed to like the idea of shortening our OpenShift builds (typically about 6.5 minutes each) but we had assumed we just needed to make a one-line change to our OpenShift template YAML, so I'll need to evaluate whether it's important enough to necessitate upstream changes or us keeping a fork that does incremental builds. It might be something the team would prefer to just abandon.

ddelabru commented 4 years ago

I don't suppose it would make sense to keep node_modules in the container image if and only if some environment variable is provided, so we can take advantage of incremental builds or choose slimmer container images instead? Would you consider merging a pull request that does this?

mprahl commented 4 years ago

I don't suppose it would make sense to keep node_modules in the container image if and only if some environment variable is provided, so we can take advantage of incremental builds or choose slimmer container images instead? Would you consider merging a pull request that does this?

Sure, that sounds good to me. My preference would be to have the default behavior keep node_modules so that way incremental builds would be intuitive to enable.

fiorenzino commented 4 years ago

Sorry, but how is it possibile? incremental builds? Can you describe, with a simple example, how we can enable that in openshift?

Best Regards

mprahl commented 4 years ago

@fiorenzino it's not currently supported with this image. Feel free to contribute this functionality though!