open-gitops / website

šŸŒ Source code for OpenGitOps website
https://opengitops.dev/
Other
34 stars 31 forks source link

Created Dockerfile and updated ReadMe #121

Closed Veinar closed 1 year ago

Veinar commented 1 year ago

Hi,

Briefly, I think i could resolve #120.

I've created basic Dockerfile that uses alpine image as base - just to keep it as lightweight as possible. Then I added an environment variable without which the build would not pass NODE_OPTIONS=--openssl-legacy-provider. In the next step, I install the necessary dependencies using the apk command, in addition, I made sure to have a git on board in case I need to download something auxiliary. Before copying application files I create workdir (gitops-website) to store it in fixed place. After that Docker will copy all files from local filesystem. I didn't create a .dockerignore file because I'm not an expert on node applications and I don't know what might not be added to the final image. (I assumed everything was needed). Lastly I expose port 8000 on which node by default exposes running application, and created entry point accordingly to ReadMe file.

I've had some issues with exposing application, and then I discovered that command: gatsby develop needs additional parameter (--host=0.0.0.0) in order to listen on all interfaces which was required to reach application from outside of docker container. So I've made changes in package.json and added this parameter into line with gatsby develop as you can see in commit. Now it is only required to pass 0.0.0.0 during port mapping in docker run.

Of course, I added information in the ReadMe file on how to build the image and how to run it.

Hope I helped, Waiting for your review, Regards, Konrad šŸ™‚

samueltauil commented 1 year ago

I would suggest changing the name of the file from Dockerfile to Containerfile as the file where you define the instructions for building an OCI-compliant container image is traditionally named Dockerfile due to its widespread use and association with Docker, however, to avoid confusion or to align with a different ecosystem, some tools or environments might opt to use a different naming convention like Containerfile, especially if they're aiming to support multiple container runtimes or be more agnostic towards the containerization framework.

I would also think of the Containerfile definition as this:

FROM node:latest

# Create a non-root user
RUN addgroup -g 1001 appuser && adduser -S -u 1001 -G appuser appuser

# Setup work directory and permissions for non-root user
WORKDIR /gitops-website
RUN chown -R appuser:appuser /gitops-website

# Switch to non-root user
USER appuser

# Add required files
COPY --chown=appuser:appuser . .

# Install dependencies
RUN npm ci --only=production

# Expose port for server
EXPOSE 8000

# Create entrypoint
CMD ["npm", "start"]
Veinar commented 1 year ago

Okay I will refer to @christianh814 questions, and take suggestions from @samueltauil in order to prepare updated version. I was using alpine image as a base which can be problematic when there is a need to specify node version.

Veinar commented 1 year ago

Hi,

I've updated PR with latest commit šŸ™‚

Now as I commented earlier will refer to questions from @christianh814,

Refering to comment from @samueltauil,

I've changed name as required šŸ™‚ and used node image as base, but I was taught to use as lightweight image as possible so I'm always trying to use image that bases on minimal image possible.

What If you have any other ideas/requests for me to change, please let me know, Have nice weekend šŸ‘‹

Regards, Konrad

P.S: using npm ci --only=production led to errors on runtime, so I had to stick with npm ci option only šŸ™‚ P.S2: I might edit default value of ARG NODE_VERSION if there will be requirement for that. It will simplify building process a bit, because it won't be needed to always pass value. Without specifying it would take default one for example 21.

niklasmtj commented 1 year ago

Hey @Veinar, thanks a lot for that PR! I'll have a look later today. One question: Did you make it run with Node19 yet? The GitHub Actions uses 16 but using 18 or 20 would be better in the near future.

Veinar commented 1 year ago

Hi @niklasmtj, thanks for response šŸ™

I'm not a professional tester, but I have successfully built containers using as a base image:

And I was able to display website using port mapping (-p 8000:8000) on my local PC šŸ™‚ As said before I'm not a tester but for me it looked fine/same (comparing as it was run on node:16-alpine). If you want I could make additional commit that will change default node version in build that --NODE_VERSION will not be required argument during build. It would rather be additional customization.

Please let me know, Regards, Konrad

P.S: Possible versions of base image could be withdrawn using this command (bash):

wget -q -O - "https://hub.docker.com/v2/namespaces/library/repositories/node/tags?page_size=1000" | jq -r '.results[] | select(.name | endswith("-alpine")) | .name' | sort -r

P.S2: To use by default latest version of node image, inc code arg value should be set to current-alpine. šŸ™‚

As for confirmation screen from running on node:21-alpine: obraz

Veinar commented 1 year ago

Hi @niklasmtj,

I made the changes based on your suggestions, thank you for picking out the node version that should be the default base for the application. In addition, I included in the comment when adding the ENV that this env should not be set when using node version 16 or lower. ā„¹ļø

As I wrote in the comment for .containerignore I would prefer to stay with the name .dockerignore due to the fact that when building an image locally via the docker or podman command - as far as I know - I will not be able to specify the name of the file with objects to ignore (for example, .containerignore) during calling the build command. Naming when using these types of files is explained here (for local docker build): https://docs.docker.com/build/building/context/#filename-and-location

Regards, Konrad šŸ™‚

Veinar commented 1 year ago

Hi team,

any update on this ? @christianh814 or @samueltauil. I don't want to be rude, but this pull request is dangling.

Regards, Konrad

samueltauil commented 1 year ago

LGTM! with a note that I still support the usage of .containerignore as all other tools support both options except for Docker.

Veinar commented 1 year ago

Thank you guys! šŸ™

Konrad

niklasmtj commented 1 year ago

Thank you guys! šŸ™

Konrad

@Veinar thank you very much for your quick help! Really appreciate it