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

initial commit for docker build #2

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jasmingacic
Copy link

@jasmingacic jasmingacic commented Nov 10, 2022

This build effectively will enable this sort of use of the CLI

docker run  --rm -ti  -v ${PWD}/basic.yml:/overlay.yaml overlay-cli

The idea is to be able to easily use overlay-cli in multiple platforms (linux, macos, win) and multiple cpu arch (arm, amd).
Ideally we could use vercel/pkg but that means packaging up to 6 different binaries into a single CLI app.

NOTE: While trying to build for platforms linux/arm/v7 llinux/arm64/v8 I found out that the official node image doesn't really work. Build was resulting in

 => ERROR [3/4] RUN npm install                                                                                                                                                                                   0.3s
------
 > [3/4] RUN npm install:
#0 0.266 exec /bin/sh: exec format error

Signed-off-by: jasmingacic [email protected]

@@ -0,0 +1,4 @@
PLATFORM?=linux/amd64

build:
Copy link
Author

Choose a reason for hiding this comment

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

possible solution would be to run npm install here

Suggested change
build:
build:
npm install


COPY . .

RUN npm install
Copy link
Author

Choose a reason for hiding this comment

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

if makefile runs npm install then there is no need for this line

Copy link
Owner

Choose a reason for hiding this comment

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

The full version of node is quite heavy, if we can get away with the alpine version, it would mean a much smaller docker image size. The only caveat is we may need to install git on the alpine version, as it does't come with that.

.dockerignore Outdated
@@ -0,0 +1 @@
node_modules
Copy link
Author

Choose a reason for hiding this comment

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

also node_modules wouldn't have to be ignored if npm install is ran with make build

Copy link
Owner

Choose a reason for hiding this comment

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

Building within the docker environment has proven to be a good approach in other projects I've worked in, as it means the host only needs Docker installed. Helps with Ci/CD pipelinles, etc.

Copy link
Owner

@ponelat ponelat left a comment

Choose a reason for hiding this comment

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

There are a few things we can do to make a production ready docker image of this. I've scattered some notes on the conversations. And can find another nodejs project that we can still the docker set up from.

Thanks for kicking this off!

.dockerignore Outdated
@@ -0,0 +1 @@
node_modules
Copy link
Owner

Choose a reason for hiding this comment

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

Building within the docker environment has proven to be a good approach in other projects I've worked in, as it means the host only needs Docker installed. Helps with Ci/CD pipelinles, etc.


COPY . .

RUN npm install
Copy link
Owner

Choose a reason for hiding this comment

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

The full version of node is quite heavy, if we can get away with the alpine version, it would mean a much smaller docker image size. The only caveat is we may need to install git on the alpine version, as it does't come with that.


RUN npm install

CMD cat overlay.yaml | node bin.js
Copy link
Owner

Choose a reason for hiding this comment

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

We can set the command to be bin.js, that way it's possible to make use of stdin and we may not need to mount files. Although mounting files may prove useful later on.

Copy link
Author

Choose a reason for hiding this comment

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

This was more of a suggestion on how to do it. I'm happy with any way you think would work the best. For example when we use this we are running command:

docker run --rm -v=${PWD}/overlay.yaml:/overlay.yaml -v=${PWD}/test.yaml:/test.yaml jasmingacic/overlay-cli

We can adjust to run it any other way.

And you are absolutely right about the docker image I used here. Current size is about 140MB. I'm not well versed with Node so I took the first thing I found.

Do you want me to add those changes?

@jasmingacic jasmingacic requested a review from ponelat November 18, 2022 13:12
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.

2 participants