paketo-buildpacks / node-run-script

Paketo Node Run Script Cloud Native Buildpack
Apache License 2.0
1 stars 7 forks source link

Feature: run build user-defined script at build phase by default #71

Closed maxgio92 closed 2 years ago

maxgio92 commented 2 years ago

What happened?

I'd like to propose to set some user-defined script to be run by default during the build phase. In example the pretty common build.

What were you attempting to do?

Build a Typescript application.

What did you expect to happen?

A build user-defined script to be run at build phase, by default without need to set environment with BP_NODE_RUN_SCRIPTS=build.

What was the actual behavior? Please provide log output, if possible.

It's needed to be set the environment variable BP_NODE_RUN_SCRIPTS with a list of script names, separated by comma (',').

Build Configuration

What platform (pack, kpack, tekton buildpacks plugin, etc.) are you using?

Please include a version.

$ pack --version
v0.23.0+git-7828226

What buildpacks are you using? Please include versions.

node-run-script:0.3.1.

What builder are you using?

If custom, can you provide the output from pack inspect-builder ?

paketobuildpacks/builder:full.

Can you provide a sample app or relevant configuration (buildpack.yml, nginx.conf, etc.)?

NONE

Checklist

g-suraj commented 2 years ago

Having npm run build executed automatically is a problem I am running into with my builder too. I am not sure which way to tackle the problem. Here are the two options I can think of

  1. Have a list of "default" commands run automatically in the build phase
  2. Have the buildpack provision a dependency and thus a custom buildpack can be written to inject whatever "default" commands are necessary in a more extensible manner.

The problem with both of these solutions is that the detect phase of node-run-script is quite tied into the assumption that the environment variable is defined

I would appreciate any insight into this issue I'm having - I wouldn't mind working on a solution, just unsure what it is at this moment.

till commented 2 years ago

It just seems that build is an established standard when I look at vue, react or svelte(kit). Not sure about others.

Couldn't the buildpack use that standard and parse package.json to verify it exists? That is, unless the user supplies BP_NODE_RUN_SCRIPTS. My hope would be though, that the user never has to learn about buildpack variables. ;)

johnnyr0x commented 2 years ago

I like this idea. Can we just supply defaults so the users don't have to learn about buildpack variables? I am thinking even beyond the run_scripts. Like default web_server=nginx and the other one below.

BP_NODE_RUN_SCRIPTS=build BP_WEB_SERVER=nginx BP_WEB_SERVER_ROOT=build

till commented 2 years ago

The BP_WEB_SERVER variables are outside of this, but this is generally a good idea, or I understand where you are coming from.

Not entirely sure though how to determine if a webserver is needed. E.g. scripts.start in your package.json could be a worker process as well. :D

till commented 2 years ago

(I edited my comment above.)

Just tried it out: https://github.com/hostwithquantum/runway-example-node

You don't even need nginx or httpd. All you need is scripts.start and for example express. Not entirely sure if this is wise to do, but it works. ;)

johnnyr0x commented 2 years ago

I missed something in my statement above. The only way the web-servers buildpack knows to use web-servers vs node.js are the flags above. If they're left out, the node.js buildpack will run.

If you know you want to build and serve the app, and use --buildpack paketo-buildpacks/web-servers but don't add those other variables in, they should have defaults to those values.

ryanmoran commented 2 years ago

@johnnyr0x, unfortunately, there's no way a buildpack can know that the user specified the --buildpack flag with a specific buildpack. During detection, the buildpack can only rely on what it sees on the filesystem and what it can see in the environment variables to make a decision about whether or not to detect.

So, why don't we just always set BP_WEB_SERVER=nginx? Well, if we did this, the Web Servers buildpack would just always detect and it would then become impossible to detect a Node.js application since its detection comes later in the order groups we ship in our builders.

Its reasonable to want better, smarter defaults for these values, but we need to take into account how those choices may impact the detection process when all buildpacks are included, such as in the Full Builder.

till commented 2 years ago

@johnnyr0x You might enjoy this discussion: https://github.com/orgs/paketo-buildpacks/discussions/32

Just to get a feel for what's possible and what others are trying.