Skip to content
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

add docker & api route #124

Closed
wants to merge 2 commits into from
Closed

add docker & api route #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f0b8ff4
add docker & api route
wvl94 Nov 13, 2024
ab656c6
docker update
wvl94 Nov 14, 2024
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
10 changes: 10 additions & 0 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
FROM node:19-alpine

WORKDIR /usr/src/app

RUN addgroup -S appgroup && adduser -S appuser -G appgroup
USER appuser

RUN apk add --no-cache git
COPY . .
CMD ["npm", "start", "--no-update-notifier"]
Comment on lines +1 to +10
Copy link

Choose a reason for hiding this comment

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

alt text

Your Dockerfile should implement multi-stage builds to optimize image size and improve build performance. Consider using a builder stage to install dependencies and build the application, then copy the necessary files to a smaller runtime image. Here's an example:

# Builder stage
FROM node:19-alpine AS builder
WORKDIR /usr/src/app
COPY package*.json ./
RUN npm install
COPY . .
RUN npm run build

# Production stage
FROM node:19-alpine
WORKDIR /usr/src/app
COPY --from=builder /usr/src/app/dist ./
CMD ["npm", "start", "--no-update-notifier"]

(Based on guideline 'Dockerfiles should use multi-stage builds')

2 changes: 1 addition & 1 deletion src/config/appConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
class Configuration {

serverPort() {
return process.env.PORT || 8080;
return process.env.PORT || 80;
}
Comment on lines +4 to 5
Copy link

Choose a reason for hiding this comment

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

alt text

You changed the default server port from 8080 to 80. This change seems important, so it might be smart to change the documentation accordingly. Update the documentation to reflect the new default port.

For example:

## Running the Application

To start the application, use:

```bash
npm start

The application will now run on port 80 by default.

logLevel(){
return process.env.LOG_LEVEL || "info";
Expand Down
3 changes: 3 additions & 0 deletions src/routes/exampleRouter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,7 @@ const router = express.Router();
router.route("/:id")
.get(grantAccessByPermissionMiddleware([API_PERMISSIONS.PUBLIC_ENDPOINT]), exampleController.getById)

router.route("/get/:id").get(exampleController.getById)

Comment on lines +12 to +13
Copy link

Choose a reason for hiding this comment

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

alt text

The route '/get/:id' is missing the grantAccessByPermissionMiddleware, which is essential for proper permission checks. Update the route definition to include this middleware as follows:

router.route("/get/:id")
  .get(grantAccessByPermissionMiddleware([API_PERMISSIONS.PUBLIC_ENDPOINT]), exampleController.getById)

(Based on guideline 'Routes should check permissions')


export default router;
Loading