[nginx]Add nginx static file planner DEV-1147 #105

mikeland73 · 2022-09-10T01:01:30Z

Summary

Implements static nginx planner for both shell and build commands. The strategy is a bit different for each:

For both shell and build nginx is installed as a package. (Maybe we should consider using services for build instead?)
For shell, we compile with a few flags to ensure nginx can run with limited commands. Some inspiration borrowed from here. It creates a script wrapper to ensure flags are correct.
For build, it's a bit more straightforward (see readme for more details). We ensure we create the nginx user following inspiration from nginx docs (we may want to follow more of that doc in the future)
Marking this as experimental, since it will probably change.

How was it tested?

unit tests
devbox shell followed by calling nginx-shell
devbox build, followed by docker run -p 80:80

LucilleH · 2022-09-12T16:37:09Z

I'm not sure if we need to support nginx in a shell. I do also have a working version with just some customization on devbox.json

mikeland73 · 2022-09-12T16:53:00Z

@LucilleH this PR is build only. But I think I can make it work in shell

mikeland73 · 2022-09-14T23:29:15Z

@LucilleH @loreto now ready for review and supports build and shell.

LucilleH · 2022-09-15T04:22:51Z

I don't know if this is the right way to solve it. I pulled the branch and tested with Docusaurus and it doesn't serve as expected (broken pages w no proper css files, etc). I might have misconfigured it perhaps.

But more importantly, do we want to use Nix services? We can generalize it to support services in general, nginx, Apache, LAMP stack, etc. See https://github.com/NixOS/nixpkgs/blob/master/nixos/tests/nginx.nix

Thoughts?

gcurtis · 2022-09-15T04:01:02Z

planner/languages/nginx/README.md

+with a few options. If you want to see what this wrapper does, use `cat $(which shell-nginx)`
+
+In shell everything is local so you should avoid pointing to assets or files outside 
+the directory because the nix shell might not have access. For example your root


Which directory?

gcurtis · 2022-09-15T04:02:39Z

planner/languages/nginx/nginx_planner.go

+}
+
+func (p *Planner) GetPlan(srcDir string) *plansdk.Plan {
+	fmt.Println(srcDir)


Intentional?

gcurtis · 2022-09-15T04:06:20Z

planner/languages/nginx/nginx_planner.go

+	nginx -c /app/%[1]s -g 'daemon off;'
+`)
+
+const customNginxDefintion = `


Suggested change

const customNginxDefintion = `

const customNginxDefinition = `

gcurtis · 2022-09-15T04:06:39Z

planner/languages/nginx/nginx_planner.go

+		// These definitions are only used in shell. Since nix is lazy, it won't
+		// actually build them at all if they are not used.
+		Definitions: []string{
+			customNginxDefintion,


Suggested change

customNginxDefintion,

customNginxDefinition,

gcurtis · 2022-09-15T04:16:49Z

planner/plansdk/plansdk.go

+// TODO: Plan currently has a bunch of fields that it should not export.
+// Two reasons why we need this right now:
+// 1/ So that individual planners can use the fields
+// 2/ So that we print them out correctly in `devbox plan`
+//
+// (1) can be solved by using a WithOption pattern, (e.g. NewPlan(..., WithWelcomeMessage(...)))
+// (2) can be solved by using a custom JSON marshaler.
 type Plan struct {


Let's put an empty line between this comment and Plan so it doesn't show up in godocs.

gcurtis · 2022-09-15T04:21:02Z

planner/plansdk/plansdk.go

@@ -116,6 +108,10 @@ func (p *Plan) WithError(err error) *Plan {
 	return p
 }

+func (p *Plan) ShellAndDevPackages() []string {
+	return pkgslice.Unique(append(p.DevPackages, p.ShellPackages...))


You probably want to copy to a new slice here, otherwise the returned slice and DevPackages will share the same backing array.

gcurtis · 2022-09-15T04:30:53Z

planner/plansdk/plansdk.go

 	DevPackages []string `cue:"[...string]" json:"dev_packages"`
+
+	// ShellPackages are only available in the shell environment.
+	ShellPackages []string `cue:"[...string]" json:"shell_packages"`


Does including shell packages in the dev environment add a lot of extra stuff?

I think trying to keep a single set of packages would be more reliable. If the shell has extra packages, then it means your dev environment no longer matches what gets built.

gcurtis · 2022-09-15T20:38:30Z

tmpl/shell.nix.tmpl

+      {{ if .ShellWelcomeMessage }}
+      echo "{{ .ShellWelcomeMessage }}"
+      {{- end }}


I know we also echo another message in this hook, but we should probably move them to the shellrc template. This hook will also run with non-interactive shells, so devbox run 'some-command' or devbox shell < myscript.sh will end up printing these messages.

mikeland73 · 2022-09-16T00:24:24Z

@LucilleH @gcurtis reimplemented using same nginx for shell and build.

The user needs to add some config options when running in shell. For simplicity I generate the file and tell them to include it in the welcome message. It's not a fantastic experience, but it prevents us from having two different nginx builds.

We could try to automate the nginx.conf construction, but I feel like that could be really error prone.

Ideally we can add the options in the command line call but I'm not sure that's possible.

LucilleH · 2022-09-16T04:22:43Z

Tested locally, when running shell-nginx I got the following error:

Starting nginx with command:
nginx -p . -c nginx.conf -e /tmp/error.log -g "pid /tmp/mynginx.pid;daemon off;"
nginx: [warn] the "user" directive makes sense only if the master process runs with super-user privileges, ignored in ./nginx.conf:1
nginx: [emerg] open() "./mime.types" failed (2: No such file or directory) in ./nginx.conf:4

LucilleH · 2022-09-16T04:28:12Z

planner/plansdk/stage.go

+}
+
+func AllFiles() []string {
+	return []string{"."}


oh this is nice. Should've done this earlier

LucilleH · 2022-09-16T04:30:57Z

Doing devbox build and docker run, I got the following error:

docker run -p 80:80 --expose 80 -ti devbox
Starting nginx with command "nginx -c /app/nginx.conf -g daemon off;"
nginx: [emerg] open() "/app/mime.types" failed (2: No such file or directory) in /app/nginx.conf:4

It was working in the previous version 🤔

LucilleH · 2022-09-16T22:40:00Z

planner/languages/nginx/nginx_planner.go

+	mkdir -p /var/cache/nginx/client_body && \
+	mkdir -p /var/log/nginx/ && \
+	PKG_PATH=$(readlink -f $(which nginx) | sed -r "s/\/bin\/nginx//g") && \
+	ln -s /app/%[1]s $PKG_PATH/conf/devbox-%[1]s && \


wow ok. I'm still getting nginx: [emerg] open() "./mime.types" failed (2: No such file or directory) in ./nginx.conf:4

nginx fails to start when the temporary directories in shell-helper-nginx.conf don't exist yet. Specifically, the `cache` subdirectory needs to be created under the temp directory. To fix this: 1. Use os.TempDir() to ensure we use the OS-specific temporary directory (which on macOS isn't `/tmp`). 2. Don't use a `cache` subdirectory so we don't need to worry about the directory existing.

gcurtis · 2022-09-19T20:45:22Z

@mikeland86 I resolved the conflicts so I could test things out. Is it okay if I push them here?

mikeland73 · 2022-09-19T21:31:39Z

@gcurtis go for it

…devbox into landau/nginx-support

gcurtis

👍 I was able to run both a shell and a built image. I think this is a good first iteration.

## Summary This is stacked on #105. Please review that one first. Adds basic pip support. * Uses requirements.txt for detection. * Uses venv for shell * Uses venv + pex for build Does not yet libraries with native extensions like `pandas` but there's a plan for that. ## How was it tested? * `devbox shell`. * `devbox build` * Unit tests Co-authored-by: Greg Curtis <greg.curtis@jetpack.io>

[nginx][WIP] Add nginx static file planner

fd0c67d

Merge branch 'main' into landau/nginx-support

9c27cc4

Lagoja linked an issue Sep 13, 2022 that may be closed by this pull request

Language Support: Nginx + Static Files #119

Closed

mikeland73 added 9 commits September 13, 2022 11:43

Merge branch 'main' into landau/nginx-support

6fc2c47

Merge branch 'main' into landau/nginx-support

ce004d7

Make nginx work on shell

c6b7e1e

Merge branch 'main' into landau/nginx-support

5d6988a

Add readme, shell config, welcome message

52ad9b3

Fix test

000f6f7

Use correct config

c59c142

Fix link

6e619d0

Bold

dbd0dbb

mikeland73 requested review from loreto, LucilleH and gcurtis September 14, 2022 23:26

mikeland73 marked this pull request as ready for review September 14, 2022 23:27

Fix test

d9b4fc1

mikeland73 changed the title ~~[nginx][WIP] Add nginx static file planner~~ [nginx]Add nginx static file planner DEV-1147 Sep 14, 2022

gcurtis reviewed Sep 15, 2022

View reviewed changes

Merge remote-tracking branch 'origin' into landau/nginx-support

23df777

gcurtis reviewed Sep 15, 2022

View reviewed changes

mikeland73 added 4 commits September 15, 2022 15:58

Use same nginx for shell and build

81d62cd

Use shellrc for welcome message

0733406

Fix test

5954fb1

Trim message

39b829d

Fix shell tests

13cc813

LucilleH reviewed Sep 16, 2022

View reviewed changes

planner/plansdk/stage.go

}

func AllFiles() []string {

return []string{"."}

Copy link

Collaborator

LucilleH Sep 16, 2022

Choose a reason for hiding this comment

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

oh this is nice. Should've done this earlier

mikeland73 added 2 commits September 16, 2022 12:33

Make sure the config runs from the nginx conf directory

7498191

Fix test

0a1e6f9

LucilleH reviewed Sep 16, 2022

View reviewed changes

gcurtis added 2 commits September 19, 2022 16:13

Merge remote-tracking branch 'origin/main' into landau/nginx-support

299d38b

mikeland73 mentioned this pull request Sep 19, 2022

[pip] Add pip support (via venv) DEV-1146 #147

Merged

mikeland73 and others added 3 commits September 19, 2022 14:45

Fix test

31dd901

Don't check GeneratedFiles when omitted in testdata

57c24cc

Merge branch 'landau/nginx-support' of https://github.com/jetpack-io/…

ec52be8

…devbox into landau/nginx-support

gcurtis approved these changes Sep 19, 2022

View reviewed changes

mikeland73 merged commit fc9c787 into main Sep 19, 2022

mikeland73 deleted the landau/nginx-support branch September 19, 2022 22:22

	const customNginxDefintion = `
	const customNginxDefinition = `

[nginx]Add nginx static file planner DEV-1147 #105

[nginx]Add nginx static file planner DEV-1147 #105

Uh oh!

Conversation

mikeland73 commented Sep 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How was it tested?

Uh oh!

LucilleH commented Sep 12, 2022

Uh oh!

mikeland73 commented Sep 12, 2022

Uh oh!

mikeland73 commented Sep 14, 2022

Uh oh!

LucilleH commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeland73 commented Sep 16, 2022

Uh oh!

LucilleH commented Sep 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucilleH commented Sep 16, 2022

Uh oh!

LucilleH Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gcurtis commented Sep 19, 2022

Uh oh!

mikeland73 commented Sep 19, 2022

Uh oh!

gcurtis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikeland73 commented Sep 10, 2022 •

edited

Loading

LucilleH commented Sep 15, 2022 •

edited

Loading

LucilleH Sep 16, 2022 •

edited

Loading