Skip to content

fix(config): generate #975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
till wants to merge 1 commit into from
Closed

fix(config): generate #975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions default_config_generator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,21 @@ func NewDefaultConfigGenerator(logs scribe.Emitter) DefaultConfigGenerator {
}

func (g DefaultConfigGenerator) Generate(config Configuration) error {
g.logs.Process("Check: %s", config.NGINXConfLocation)
Comment thread
dmikusa marked this conversation as resolved.
if info, err := os.Stat(config.NGINXConfLocation); err == nil {
g.logs.Subprocess("Configuration file already exists")
if info.Size() > 0 {
userConf, err := os.ReadFile(config.NGINXConfLocation)
if err != nil {
return err
}

g.logs.Subprocess("Set configuration (in %s) as template", config.NGINXConfLocation)

@dmikusa dmikusa Aug 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 till Aug 26, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 till Aug 26, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 till Aug 26, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand what you are saying.

@dmikusa dmikusa Aug 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Aug 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 till Aug 26, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made another PR but there are weird test errors.


DefaultConfigTemplate = string(userConf)
}
}

g.logs.Process("Generating %s", config.NGINXConfLocation)
t := template.Must(template.New("template.conf").Delims("$((", "))").Parse(DefaultConfigTemplate))

Expand Down
47 changes: 47 additions & 0 deletions default_config_generator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,53 @@ error_log stderr;
`)))
})

context("existing nginx.conf, BP_WEB_SERVER=nginx", func() {
it("doesn't overwrite an existing nginx.conf", func() {
confPath := filepath.Join(workingDir, "nginx-exists-non-empty.conf")
Expect(os.WriteFile(confPath, []byte("port {{port}};"), 0644)).To(Succeed())

Expect(generator.Generate(nginx.Configuration{
NGINXConfLocation: confPath,
WebServer: "htdocs",
})).To(Succeed())

Expect(confPath).
To(matchers.BeAFileMatching(ContainSubstring(string("port {{port}}"))))

Expect(os.Remove(confPath)).To(Succeed())
})

it("sets variables inside the supplied nginx.conf", func() {
confPath := filepath.Join(workingDir, "nginx-exists.conf")
Expect(os.WriteFile(confPath, []byte("root $(( .WebServerRoot -));"), 0644)).To(Succeed())

Expect(generator.Generate(nginx.Configuration{
NGINXConfLocation: confPath,
WebServerRoot: "/a/very/specific/path",
})).To(Succeed())

Expect(confPath).
To(matchers.BeAFileMatching(ContainSubstring("root /a/very/specific/path;")))

Expect(os.Remove(confPath)).To(Succeed())
})

it("overwrites an empty nginx.conf", func() {
confPath := filepath.Join(workingDir, "nginx-exists-empty.conf")
Expect(os.WriteFile(confPath, []byte(""), 0644)).To(Succeed())

Expect(generator.Generate(nginx.Configuration{
NGINXConfLocation: confPath,
})).To(Succeed())

// check that there's anything inthere
Expect(confPath).
To(matchers.BeAFileMatching(BeEquivalentTo(`root {{ env "APP_ROOT" }};`)))

Expect(os.Remove(confPath)).To(Succeed())
})
})

context("failure cases", func() {
context("destination file already exists and it's read-only", func() {
it.Before(func() {
Expand Down
Loading