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.

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

feat(KUI-1542): update dependencies (upgrade node version from 18 to 20) & improve the code for running locally using docker #390

Merged
merged 7 commits into from
Dec 9, 2024

Conversation

amirhossein-haerian
Copy link
Contributor

@amirhossein-haerian amirhossein-haerian commented Nov 22, 2024

The main purpose of this PR is to upgrade the node version from 18 to 20 and then update the dependencies of this Repo.

The issue with running the project locally using dockerfile found while updating the node version to 20 and I decided to upgrade the repo to be able to run the project in dev mode using docker.

A script is added to fetch all environment variables.

Environment variables

The required environment variables to run this project for development are listed in the .env.in file. You can retrieve their corresponding values from the Azure portal and populate the environment variables accordingly. To fetch all the environment variables, use the following command:

  • Ensure you are logged in to Azure before running this command. You can log in by using the az acr login command.

  • For running this command you need to be logge in on Azure for this you can run az acr login command

I also added this description for how we can use docker to run the project.

Run Docker Container

After the image has been built, you can start the Docker container using the following command:

npm run docker:run

This will execute the docker-run-image.sh script in development mode (dev), running the application locally in Docker.

The application now will be accessible at http://localhost:3000/student/kurser/kurs/:courseCode.

  • To run this project locally, ensure all required environment variables are set. You can do this by running the command: npm run fetch-all-env-variables.

Alternative approach for running locally using Docker

alternatively you can run the following command:

docker-compose up

Here you need to remove the .in at the end of the docker-compose.yml.in.

  • Run az acr login --name kthregistry before running the scripts.

Copy link
Contributor

@karlandindrakryggen karlandindrakryggen left a comment

Choose a reason for hiding this comment

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

I was a little confused about the docker setup. I left a long comment below, but I guess the question really is why you need to start the docker in "dev" mode?

I'm not sure if and how I want it to be changed, but I vote "Request changes" now cause I want you to have look at it again, and maybe you/we should discuss in the team how we want this to be setup.

docker-build-image.sh Outdated Show resolved Hide resolved
package.json Outdated
@@ -14,6 +14,7 @@
"build-dev": "bash ./build.sh dev",
"docker:build": "bash ./docker-build-image.sh dev",
"docker:run": "bash ./docker-run-image.sh dev",
"docker:start": "bash -c 'cat /KTH_NODEJS; NODE_ENV=development node app.js'",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about the naming here. The only difference between "docker:start" and just "start" is that NODE_ENV is development instead of production. I was a little confused about this, because there is nothing docker-related by this at all. I understand that you are using this in a dockerfile, but the regular "start" is also use in a dockerfile.

My first thought was to just rename this one to "start-dev", but then I realised we already have a "start-dev" for other purposes.

But maybe the bigger question maybe is: Why do we need to start it in dev mode? I think that one of the big advantages of running in docker here would be to test it as close as possible to a real "production" environment. What benefits do we get from starting with node_env=dev here?

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'm not sure about the naming here. The only difference between "docker:start" and just "start" is that NODE_ENV is development instead of production. I was a little confused about this, because there is nothing docker-related by this at all. I understand that you are using this in a dockerfile, but the regular "start" is also use in a dockerfile.

My first thought was to just rename this one to "start-dev", but then I realised we already have a "start-dev" for other purposes.

But maybe the bigger question maybe is: Why do we need to start it in dev mode? I think that one of the big advantages of running in docker here would be to test it as close as possible to a real "production" environment. What benefits do we get from starting with node_env=dev here?

It is great that you mentioned it,
I shared my thoughts in the other comment regarding the dev mode for running docker.

regarding the new script that I have added I was thinking to make a script only for docker in dev mode but then I thought using start-dev would be better than creating a new one.

Thanks for your comment ⭐️

@karlandindrakryggen
Copy link
Contributor

Also just a comment about the PR title and description. The "real" change introduced in this PR is the upgrade from node 18 to 20, so I think that is the most important thing to include in the title/description.

@amirhossein-haerian
Copy link
Contributor Author

amirhossein-haerian commented Dec 2, 2024

I was a little confused about the docker setup. I left a long comment below, but I guess the question really is why you need to start the docker in "dev" mode?

I'm not sure if and how I want it to be changed, but I vote "Request changes" now cause I want you to have look at it again, and maybe you/we should discuss in the team how we want this to be setup.

Thanks for this comment ⭐️,

I agree that changing the docker-build-image.sh may add complexity and confusion.

The general issue here was that docker-build-image.sh file were using Dockerfile-dev which did not exist, so first I generated one and I saw that the only difference was the last line which was using docker:start instead of start then I decided to automate this generation in case that the main Dockerfile changed the Dockerfile-dev also changes accordingly.

But now that I am thinking maybe this is not necessary to automate this process and only adding the Dockerfile-devwould be enough. So I made the required changes inside docker-build-image.sh and basically convert that to what is was before.

Regarding running the docker in dev mode I would say the approach in other repos is the same as well and I think we should follow the same approach.

I realized that for web repos we have the same issue with Redis when we run the docker in production mode, it may be fixed by using production environment variables but I believe we should avoid using production environment variables for test scenarios. (It is possible to run the docker in production mode when it is really needed and in very unique scenarios)

My personal thought here is that the greatest advantage of running the Dockerfile in dev mode would be that we can replicate the process of publishing our project and see if there is any issue in building the process using Docker without using the production environment variables.

After all, It would be amazing if we have a discussion regarding this as well.

@amirhossein-haerian
Copy link
Contributor Author

Also just a comment about the PR title and description. The "real" change introduced in this PR is the upgrade from node 18 to 20, so I think that is the most important thing to include in the title/description.

I completely agree with you and sorry for that, I will change the PR title and description.

@amirhossein-haerian amirhossein-haerian changed the title feat(KUI-1542): improve the code for running locally using docker & update dependencies feat(KUI-1542): update dependencies (upgrade node version from 18 to 20) & improve the code for running locally using docker Dec 2, 2024
@karlandindrakryggen
Copy link
Contributor

My personal thought here is that the greatest advantage of running the Dockerfile in dev mode would be that we can replicate the process of publishing our project and see if there is any issue in building the process using Docker without using the production environment variables.

I agree 100% with this, that you want to replicate the process of publishing. And this is why I is a little bit skeptical to run this in dev mode.

Note that "node environment"=production isn't same as running with environment variable value from our production env. The ref environment is running in node env=production as well.

@amirhossein-haerian
Copy link
Contributor Author

My personal thought here is that the greatest advantage of running the Dockerfile in dev mode would be that we can replicate the process of publishing our project and see if there is any issue in building the process using Docker without using the production environment variables.

I agree 100% with this, that you want to replicate the process of publishing. And this is why I is a little bit skeptical to run this in dev mode.

Note that "node environment"=production isn't same as running with environment variable value from our production env. The ref environment is running in node env=production as well.

Great that you mentioned that, in that case we need to investigate why we have the issue with Redis when we set the NODE_ENV to "development".

@amirhossein-haerian
Copy link
Contributor Author

amirhossein-haerian commented Dec 3, 2024

Based on our session, I tried to fetch all environment variables.
I could write a script that will fetch all of the environment variables and put them inside .env file.

After having all the environment variables, I tested the npm run docker:build and npm run docker:run which worked.

@amirhossein-haerian amirhossein-haerian merged commit 59a0fec into master Dec 9, 2024
3 checks passed
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.

3 participants