npm: add basic example from sample-nodejs-app#3
npm: add basic example from sample-nodejs-app#3derasdf wants to merge 2 commits intohermetoproject:npm-basicfrom
Conversation
Summary of ChangesHello @derasdf, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive example demonstrating the integration of Hermeto with a Node.js application using npm for dependency management. The primary goal is to illustrate how to achieve reproducible and offline builds by leveraging Hermeto's capabilities to manage and inject dependencies, ensuring that the application can be built within a container without external network access for package resolution. This addition enhances the documentation with a practical, runnable scenario for npm users. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a basic npm example. The changes are generally good, but I've identified several areas for improvement regarding Docker best practices, documentation clarity, and Node.js project conventions. My comments include suggestions to optimize the Docker build, enhance security by running as a non-root user, correct the example's usage instructions, fix a typo, and add a standard start script.
| RUN npm install --verbose | ||
|
|
||
| EXPOSE 9000 | ||
|
|
There was a problem hiding this comment.
For security, it's a best practice to run containers with a non-root user. The official node images include a node user for this purpose. You can switch to this user here before the CMD instruction.
USER node
| COPY . . | ||
|
|
||
| RUN npm install --verbose |
There was a problem hiding this comment.
To optimize Docker build times, you should leverage layer caching for dependencies. The current setup copies all files before running npm install, which means any code change will invalidate the cache for the npm install layer.
A better approach is to copy only the package.json and package-lock.json files, install dependencies, and then copy the rest of the application code.
I'd suggest changing this part of the Containerfile to:
COPY package*.json ./
RUN npm install --verbose
COPY . .| podman build . \ | ||
| --volume "$(realpath ./hermeto-output)":/tmp/hermeto-output:Z \ | ||
| --volume "$(realpath ./hermeto.env)":/tmp/hermeto.env:Z \ | ||
| --network none \ | ||
| --tag npm-basic-example | ||
| ``` |
There was a problem hiding this comment.
The podman build command mounts hermeto.env, but the Containerfile doesn't use it. The RUN npm install step won't pick up any environment variables from that file.
To make this example work as intended (especially if hermeto.env contains variables in the future), the Containerfile should be modified to source this file. For example:
RUN . /tmp/hermeto.env && npm install --verbose
Since the README states that this file might be used in the future, the example should demonstrate its correct usage.
index.js
Outdated
|
|
||
| app.get("/", (req, res) => res.send("Hello Dude!")); | ||
|
|
||
| app.listen(port, () => console.log(`App started o port ${port}...`)); No newline at end of file |
There was a problem hiding this comment.
There's a typo in the log message ('o' should be 'on'). Also, it's a common convention to end files with a newline character.
| app.listen(port, () => console.log(`App started o port ${port}...`)); | |
| app.listen(port, () => console.log(`App started on port ${port}...`)); | |
| "description": "", | ||
| "main": "index.js", | ||
| "scripts": { | ||
| "test": "echo \"Error: no test specified\" && exit 1" |
There was a problem hiding this comment.
It's a good practice to add a start script to define how to run the application. This is idiomatic in Node.js projects and makes it easier for others to run the app. You could then change the CMD in your Containerfile to CMD ["npm", "start"].
"start": "node index.js",
"test": "echo \"Error: no test specified\" && exit 1"
a-ovchinnikov
left a comment
a-ovchinnikov
left a comment
There was a problem hiding this comment.
LGTM: migration comes first, cleanup after that (a typo in a message is safe to fix now though).
Migrate the basic npm sample from cachito-testing/sample-nodejs-app to doc-examples and adapt it to use Hermeto for npm dependencies. Signed-off-by: Vladimir Aleksandrov <valeksan@redhat.com>
Signed-off-by: Vladimir Aleksandrov <valeksan@redhat.com>
2 participants
Migrate the basic npm sample from cachito-testing/sample-nodejs-app to doc-examples and adapt it to use Hermeto for npm dependencies.