-
Notifications
You must be signed in to change notification settings - Fork 14
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.
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
#88 - Analysis to Migrate the Vue Server #2284
Conversation
COPY nginx.conf /etc/nginx/nginx.conf | ||
|
||
# Daemon off makes nginx to run on the foreground with only one process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
sources/packages/web/nginx.conf
Outdated
try_files $uri $uri/ /index.html; | ||
} | ||
error_page 500 502 503 504 /50x.html; | ||
location = /50x.html { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for my understanding, what is /50x.html
. is there an HTML file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the html file for the 50x errors. We don't have one. Should we create a place holder one or remove that configuration for now? Ideas? @dheepak-aot @guru-aot @andrewsignori-aot @sh16011993
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be ok to have it removed. We can track improvements in a new ticket to be created.
Good work @andrepestana-aot 👍 Just some minor comments and a question |
@@ -153,6 +153,11 @@ api: ## <Helper> :: Executes into the workspace container. | |||
@echo "+\n++Shelling into local workspace ...++\n+" | |||
@docker-compose exec api bash | |||
|
|||
# Local Web (nginx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @andrepestana-aot , we can have a discussion on the nginx configuration with the team and can close it.
# Daemon off makes nginx to run on the foreground with only one process. | ||
# Docker will kill the container if the process dies. | ||
CMD ["nginx", "-g", "daemon off;"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Please take a look at the comments.
The below BC Gov app has an nginx config com a section named "Headers required by Web App Vulnerability Scan". Maybe that is something else that we will need to look into in a separate ticket. After this effort, we can request a new Wava Scan to get an updated report.
https://github.com/bcgov/moh-hnweb/blob/main/frontend/.nginx/nginx.conf
sources/packages/web/Dockerfile.dev
Outdated
RUN npm run build-local | ||
|
||
# Get a nginx image to have only the compiled app ready for production. | ||
FROM nginx:1.22 AS production-stage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor but can we make the layer name production-stage
aligned with the other Dockerfile. Mentioning production here and in the line 24 comment can be misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes and for the research. I left just one minor comment as a suggestion, hence approving it. Looks good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Good work with the analysis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank You for the analysis and migration to nginx.
Kudos, SonarCloud Quality Gate passed!
|
|
||
COPY ./package*.json ./ | ||
RUN npm ci | ||
COPY . ./ | ||
|
||
# Replace ${PORT} variable in the template and save as default.conf. | ||
RUN sed 's/${PORT}/'"${PORT}"'/g' default.conf.template > default.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it not the same as having the PORT hardcode directly at the file?
Just wondering what is the benefit between having it hardcoded in the file vs the like 4.
Either way it is just a question for my information, not a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Dockerfile.dev it uses 8080 but for Dockerfile it uses 3030. We can change that later. It's just that in some places it has 3030 as the default port. We can change our local to 3030 too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work put in to move to NGINX.
Using npm
serve
package or vue-cli to host a Vue.js app in a production environment is generally not recommended for several important reasons like limited performance, security concerns, lack of Load Balancing, low customization, inefficient static file serving, bad redundancy and scalability and lack of features for monitoring and logging.Choosing a web server
According to w3techs and Netcraft websites Nginx is the most used web server at the moment:

Among the benefits of adopting Nginx for the project the items below can be highlighted:
During the analysis it's been noticed an increasing number of projects using Caddy Server among the BCGov projects.
Although there isn't any direction to use Nginx in BC Gov projects, it's widely used and, as mentioned above, there are several reasons why we should use Nginx in SIMS web app.
Docker build recommendations:
Configuration
The configuration for the webserver is setup on nginx.conf and it is set with the suggested conf from vue-cli docs and a few tweaks. A full example with configuration options can be found here.
To use variables in nginx configuration it's needed to use
envsubst
and templating.Notes
process
is not set and thebasesUrl
for the app is set as in "production" mode.Next steps:
E.g.:
References