Skip to content

Conversation

@gortiz
Copy link
Contributor

@gortiz gortiz commented Feb 2, 2023

pinot-controller contains a resource which is the controller frontend, which is built with npm. This PR tries to improve the developer relation with these code by:

  1. Using npm ci instead of npm install.
  2. Using webpack --mode development instead of webpack --mode production by default.

The first is a recommended way to build npm projects when package-lock.json is used. From https://docs.npmjs.com/cli/v9/commands/npm-ci:

This command is similar to npm install, except it's meant to be used in automated environments such as test platforms, continuous integration, and deployment -- or any situation where you want to make sure you're doing a clean install of your dependencies.

The differences with npm install can be seen in the link above, but the main idea is that npm ci will always use the dependencies in package-lock.json while npm install will try to verify if there is a newer library version (when open versions, aka ^ in npm, are used). By using npm ci we are going to have more repeatable builds. The idea is that whenever we actually want to upgrade dependencies we should do npm install (or the alternative that only updates the single dependency we care about) locally, which will modify the package-lock.json and the commit that file, which will make that any npm ci executed after that (in both dev envs and ci env) will use the new dependency.

The second change tries to accelerate the build in dev machines. Right now the generate-resources maven phase is bound to execute npm run-script build, which in fact executes webpack --mode production. There is another npm script called build-dev which runs webpack --mode development. The difference between these modes can be seen in the offical documentation, but the idea is that development mode should be used in... well, development environments. It will not mangle the code, which means that it will be more verbose and easier to debug, but it will take less time to create the package.

What this PR does is to add and use a new npm script called build-ci which is defined as:

    "build-ci": "if [ \"$CI\" = true ]; then npm run-script build; else npm run-script build-dev; fi",

CI is a variable defined in most CIs, including GitHub Actions (link). If this variable is defined and its value is true, then npm run-script build will be executed, which is the normal behavior right now. Otherwise npm run-script build-dev will be executed. Therefore in dev machines, where CI env variable will not be defined, we will build the bundle using the faster and easier to debug mode.

In the build log we can know whether it is using one mode or the other. For example:

[INFO] > if [ "$CI" = true ]; then npm run-script build; else npm run-script build-dev; fi
[INFO] 
[INFO] 
[INFO] > pinot-controller-ui@1.0.0 build /opt/proyectos/startree/pinot/pinot-controller/src/main/resources
[INFO] > webpack --mode production

This PR also changes the dockerfile when building Pinot in order to set the CI variable to true by default. This is needed to correctly build the image in both CI environments and in local ones, although we can opt out by setting the docker ARG to false

@Jackie-Jiang Jackie-Jiang merged commit 4c05a37 into apache:master Feb 2, 2023
<properties>
<pinot.root>${basedir}/..</pinot.root>
<npm.script>build</npm.script>
<npm.script>build-ci</npm.script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we undo this change. This has effectively made it so everyone building this project outside of that one Dockerfile is actually building dev. I randomly caught this on one of our clusters because we (and probably others) build from source ourselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH that was the intention. The idea is that unless you want to build a production bundle, you should build the dev version.

I don't think we need to rollback this, but what we can do is to run npm build when profile bin-dist is enabled, as that is the maven way to say that we want to have a production bundle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed this in #10525

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants