PHP-FPM options via environment variables

[iamacarpet]

From the conversation we've had in #392 , here is a replacement change for configuring php-fpm via environment variables, e.g.:

PHP_FPM_DYNAMIC_WORKERS=true
PHP_FPM_WORKER_COUNT=8

Based on @sonnysasaka 's branch for adding NGINX_DOCUMENT_ROOT, as I think it's important they go together.

@sonnysasaka @sl0wik are you both happier with this one?

And @jama22 @kennethye1 , would you be happy to merge this if the other two are happy please?

Regards,
iamacarpet

[sl0wik]

Looks good to me.

[sonnysasaka]

nit: not a big deal, but I would simplify this to return 2 values instead of 3:

func GetIfInt(varName string) (int, bool) {
...
}

We lost the error indication, but simplify things a bit. Your call.

[iamacarpet]

To be honest, I initially wrote it this way then backtracked, as everywhere else is very heavy on catching the errors & throwing them properly, so I didn't want to implement something that would silently fail (and revert to the default) if someone makes a mistake in the environment variable and it doesn't parse properly.

I'm happy either way though - if someone from Google can weigh in on which one you'd be more likely to accept for merge, I'll make it so.

[sonnysasaka]

How did you test this? Not sure if the acceptance test framework is ready for this configuration.

@kennethye1 any idea how to test this? In general, how do we build the builder image so we can test manually? (I haven't done that before since I relied on the acceptance test for past contribution)

[iamacarpet]

How did you test this? Not sure if the acceptance test framework is ready for this configuration.

I haven't been able to test this so far, as I'm not sure how.

And likewise, I couldn't see any existing tests that could be used as a template to evaluate the output for the php-fpm config.

Any guidance would be greatly appreciated.

[kennethye1]

I believe our CONTRIBUTING.md has the info. In particular, I think the command bazel build //builders/gcp/base:google_22_builder.image

[sonnysasaka]

Thanks @kennethye1 for the pointer. Tried to follow CONTRIBUTING.md but it doesn't work:

$ bazel build builders/gcp/base:builder.image
ERROR: /home/sonny/projects/buildpacks/cmd/go/functions_framework/BUILD.bazel:8:10: Label '//cmd/go/functions_framework:converter/without-framework/main.go' is invalid because 'cmd/go/functions_framework/converter/without-framework' is a subpackage; perhaps you meant to put the colon here: '//cmd/go/functions_framework/converter/without-framework:main.go'?
ERROR: /home/sonny/projects/buildpacks/cmd/go/functions_framework/BUILD.bazel:8:10: Label '//cmd/go/functions_framework:converter/without-framework/go.mod' is invalid because 'cmd/go/functions_framework/converter/without-framework' is a subpackage; perhaps you meant to put the colon here: '//cmd/go/functions_framework/converter/without-framework:go.mod'?
ERROR: /home/sonny/projects/buildpacks/builders/gcp/base/BUILD.bazel:7:8: Target '//cmd/go/functions_framework:functions_framework.tgz' contains an error and its package is in error and referenced by '//builders/gcp/base:builder_go'
ERROR: Analysis of target '//builders/gcp/base:builder.image' failed; build aborted: Analysis failed

Not a problem to solve currently (maybe just outdated doc?), because I did some trial and error and found that bazel build builders/php/builder.image is the command that works and produces a local docker image gcp/php and I was able to use for manual testing.

So I was able to test manually that this PR works as intended this way: https://github.com/sonnysasaka/buildpack-demo/blob/c702b475f6583db3d07e3b8b64f474eea001d28e/build.sh, except that this PR change is missing a dependency @iamacarpet:

diff --git a/pkg/webconfig/BUILD.bazel b/pkg/webconfig/BUILD.bazel
index 32fb0a09..7a8a2bba 100644
--- a/pkg/webconfig/BUILD.bazel
+++ b/pkg/webconfig/BUILD.bazel
@@ -9,6 +9,7 @@ go_library(
     ],
     deps = [
         "//pkg/appyaml",
+        "//pkg/env",
         "//pkg/gcpbuildpack",
         "//pkg/php",
         "@com_github_buildpacks_libcnb//:go_default_library",

@iamacarpet please add the missing dependency, and if @kennethye1 is fine with the code I support this change and have verified manually that it works as intended.

[iamacarpet]

Thanks so much for helping me navigate the testing & the bazel dependencies @sonnysasaka .

Agree with your call on the function name, thanks for pointing it out!

That should all be in the latest change - thanks for all the help everyone.

Might want to squash merge this one after I've had to add the additional commits - sorry about that!

[sonnysasaka]

overall LGTM from me and verified working. I recommend squashing the changes into 2 commits (one authored by me about the document root and another one by @iamacarpet about the FPM configs + some refactor). @kennethye1 to weigh in.

[iamacarpet]

I recommend squashing the changes into 2 commits (one authored by me about the document root and another one by @iamacarpet about the FPM configs + some refactor).

Very good point @sonnysasaka , you can tell my mind is busy on other things, shame on me!

Thanks for pointing this out - I've squashed my own commits into a single commit, keeping your commit first.

[kennethye1]

IMO it seems like we are adding a lot of environment variables vs having the user provide their own php-fpm file.

[sonnysasaka]

@kennethye1 I understand that it's almost as easy to just provide the php-fpm.conf. How about the "document root" one? Currently there is no easy way to configure this, without replacing the whole nginx.conf which is hard for user. Are you okay with adding NGINX_DOCUMENT_ROOT env var (also part of this PR and originally from #390)