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

Created Dockerfile and updated ReadMe #121

Merged
merged 3 commits into from
Nov 20, 2023
Merged

Created Dockerfile and updated ReadMe #121

merged 3 commits into from
Nov 20, 2023

Conversation

Veinar
Copy link
Contributor

@Veinar Veinar commented Nov 3, 2023

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 🙂

@@ -9,7 +9,7 @@
],
"scripts": {
"develop": "gatsby develop",
"start": "gatsby develop",
"start": "gatsby develop --host=0.0.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

How does this æffect running it in production? (if at all) @niklasmtj

Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't effect it at all since we're pushing the public folder to GH pages

Dockerfile Outdated

# Expose port for server
EXPOSE 8000

Copy link
Member

Choose a reason for hiding this comment

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

Talked to @samueltauil about this. I would rather not run it as root if at all possible.

I would suggest adding something like

RUN chown -R 1001:0 /gitops-website && chmod -R ug+rwx /gitops-website
USER 1001

Copy link
Member

@niklasmtj niklasmtj Nov 3, 2023

Choose a reason for hiding this comment

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

Maybe we can even use node:16-alpine? This way we could use the node user

Copy link
Member

@christianh814 christianh814 left a comment

Choose a reason for hiding this comment

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

In general I like this. But I have a few questions concerns before I can approve.

  • Does this emulate the env that the site is running on? (I'm mainly concerned about the version of node)
  • The Container would run as root. Which isn't terrible if you're just developing locally but we should probably do the best practice here.
  • Looks like we need to change the package.json file. What is the implications?
  • DCO isn't passing.

@samueltauil
Copy link

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
Copy link
Contributor Author

Veinar commented Nov 4, 2023

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
Copy link
Contributor Author

Veinar commented Nov 4, 2023

Hi,

I've updated PR with latest commit 🙂

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

  • Does this emulate the env that the site is running on? (I'm mainly concerned about the version of node)

I do not have information about current setup, but regarding node version I've made changes within Containerfile where you now specify NODE_VERSION in build which selects version from node docker image.

  • The Container would run as root. Which isn't terrible if you're just developing locally but we should probably do the best practice here.

Yes totally agree, changed to built-in user node. Slightly different than @samueltauil proposed in his draft but It might be enough.

  • Looks like we need to change the package.json file. What is the implications?

To the best of my knowledge, there will be no implications, because adding --host=0.0.0.0 only sets the listener to a wider range. A similar solution is used, for example, if you want to reach an application from a local network from a second device. This should be considered more as a door ajar a bit more rather than a change that will affect the operation of the application.

  • DCO isn't passing.

Yes I am aware of that, and it is caused because my first commit was not signed, in second one I've corrected myself but used SSH key, wthen I had to process procedure to signoff commits in batch but it is done ✔️

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
Copy link
Member

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
Copy link
Contributor Author

Veinar commented Nov 8, 2023

Hi @niklasmtj, thanks for response 🙏

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

  • 16-alpine ✔️
  • 18-alpine ✔️
  • 19-alpine ✔️
  • 20-alpine ✔️
  • 21-alpine ✔️

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

Copy link
Member

@niklasmtj niklasmtj left a comment

Choose a reason for hiding this comment

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

Hey Konrad (@Veinar),

This looks like a great addition. Thanks again for this!
I got a few comments and suggestions on your changes after testing it locally. Still, I'm pretty happy with these changes!

Containerfile Outdated
@@ -0,0 +1,32 @@
# Use the NODE_VERSION argument to specify the Node.js version
ARG NODE_VERSION
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ARG NODE_VERSION
ARG NODE_VERSION=20

I would suggest setting a default value here so we can also build it without the build argument if we want. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Ah before i forget. I used node-20 since this is the latest LTS version

Containerfile Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated
docker build --build-arg NODE_VERSION=19 --no-cache -t website:<tag> -f Containerfile .

# Run container image with mapping port 80 on your computer
docker run -dit -p 80:8000 website:<tag>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
docker run -dit -p 80:8000 website:<tag>
docker run -dit -p 8000:8000 website:<tag>

I understand why you default to 80 here and it also makes a lot of sense. What I think is that this might confuse folks since the console output for example says the following:

You can now view open-git-ops in the browser.
⠀
  Local:            http://localhost:8000/

Containerfile Show resolved Hide resolved
@niklasmtj niklasmtj added the enhancement New feature or request label Nov 8, 2023
@Veinar
Copy link
Contributor Author

Veinar commented Nov 8, 2023

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 Veinar requested a review from niklasmtj November 8, 2023 22:28
Copy link
Member

@niklasmtj niklasmtj left a comment

Choose a reason for hiding this comment

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

LGTM. Let's see what the other maintainers say ☺️

Thank you very much Konrad!

@Veinar Veinar requested a review from samueltauil November 9, 2023 13:31
@Veinar
Copy link
Contributor Author

Veinar commented Nov 20, 2023

Hi team,

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

Regards,
Konrad

Copy link
Member

@christianh814 christianh814 left a comment

Choose a reason for hiding this comment

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

LGTM!

@samueltauil
Copy link

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

@niklasmtj niklasmtj merged commit a61e888 into open-gitops:main Nov 20, 2023
1 check passed
@Veinar
Copy link
Contributor Author

Veinar commented Nov 21, 2023

Thank you guys! 🙏

Konrad

@niklasmtj
Copy link
Member

Thank you guys! 🙏

Konrad

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Local Docker setup
4 participants