fix(config): generate

[till]

this commit ensures that an existing nginx.conf is not overwritten by
the config generator of the buildpack. for cases where a consumer of
the buildpack used an empty nginx.conf to trigger inclusion of the
buildpack in the plan, the generator will generate a full nginx.conf
according to the buildpack's defaults.

related: #667

Summary

Use Cases

Checklist

• I have viewed, signed, and submitted the Contributor License Agreement.
• I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
• I have added an integration test, if necessary.
• I have reviewed the styleguide for guidance on my code quality.
• I'm happy with the commit history on this PR (I have rebased/squashed as needed).

[dmikusa]

I don't think we should do this, it's changing the functionality here. The template previously was hard coded, and the intent of this buildpack was that the initial generated configuration would always be simple & if you out grow it, then you need to just supply your own config file. If we go down the path of allowing custom templates, that feels like it opens up a lot of area of things we need to support.

[till]

I understand. I guess reading the example, I was (probably wrongfully) assuming that the full config capabilites are possible: https://github.com/paketo-buildpacks/samples/blob/main/web-servers/nginx-sample/nginx.conf

E.g., {{port}} is supported, but not the rest?

[till]

@dmikusa would you be in favour of not overwriting the configuration file though when present? I think that is a "bug"? Or is this intended?

[till]

I think I understand what you are saying.

[dmikusa]

Ah, ok. Yes, that is a confusing bit.

The {{port}} configs are actually resolved at runtime, see here. So that's different configuration & we do process all nginx.conf files at runtime, whether they are auto-generated or supplied by a user.

The template config that happens in your PR is when we generate a configuration file because there is no nginx.conf file at all. It's meant to be the "quick start" option for some standard use cases like serving up files or a SPA app. This is where things like BP_WEB_SERVER_ENABLE_PUSH_STATE and the other BP_WEB_SERVER_* config comes into play. See here. What I was saying above only applies to this config generation.

There are an almost infinite number of ways you can configure nginx. In a previous buildpack, we tried to be very flexible and support a ton of different config options. What ended up happening is that we just invented another (arguably worse) way to configure Nginx. With this buildpack, we mad the decision to limit what the auto generation would do to a few common use cases. That way users can quickly get started with those common cases, and when/if they outgrow what that can do, then they can use standard Nginx config to accomplish their goals (not some special thing only applicable to buildpacks).

[dmikusa]

@dmikusa would you be in favour of not overwriting the configuration file though when present? I think that is a "bug"? Or is this intended?

Yes, my $0.02 is that the bug here is not checking if the file exists. We should not overwrite a user supplied config file. I think we can just add a check to make sure it doesn't exist and that should solve this issue. If you supply a file, even if it's empty, then we'll attempt to use that.

That said, I'd be OK generating a separate error if you supply an empty nginx.conf file. That's pretty obviously not going to work. I just think that's a separate thing from the generation of an nginx.conf file.

[till]

I made another PR but there are weird test errors.

[till]

#976