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

Add docker containers and release actions #21

Merged
merged 3 commits into from
Oct 15, 2024
Merged

Add docker containers and release actions #21

merged 3 commits into from
Oct 15, 2024

Conversation

uncleDecart
Copy link
Owner

No description provided.

@uncleDecart
Copy link
Owner Author

Solves #16, we will have to merge and create release to debug GHA

Copy link
Collaborator

@deitch deitch left a comment

Choose a reason for hiding this comment

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

Is there any reason not to have a single container image with both binaries in it? They are pretty small, it gives you everything in a single image, makes the repo cleaner with a single Dockerfile.

I am pretty sure etcd, redis, mysql, mariadb, postgres, etc. all do it the same way.

@uncleDecart
Copy link
Owner Author

Then your entry point will be shell and then you write nkv-server "0.0.0.0:4222" or nkv-client "127.0.0.1:2222", okay that makes sense, I'll group it all in :) Thanks, Avi!

@uncleDecart uncleDecart force-pushed the add-docker branch 2 times, most recently from 9b690ae to 11e9415 Compare October 8, 2024 16:26
@uncleDecart
Copy link
Owner Author

@deitch this too

@uncleDecart
Copy link
Owner Author

Rebased to master so that Cargo.lock is a valid file

Copy link
Collaborator

@deitch deitch left a comment

Choose a reason for hiding this comment

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

Some questions and a suggestion.

Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
README.md Outdated
docker run -it --net=host uncledecart/nkv:latest ./nkv-client "4222"
```

Note that when using network different from host you might face major performance degradation, also docker
Copy link
Collaborator

Choose a reason for hiding this comment

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

That is a bit harsh. Maybe tone it down?

when using network other than the host, the network implementation may impact network performance, unrelated to nkv itself.

It includes publishing container to DockerHub

Signed-off-by: Pavel Abramov <[email protected]>
@uncleDecart
Copy link
Owner Author

@deitch addressed the comments

@uncleDecart uncleDecart merged commit e0f81df into main Oct 15, 2024
2 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.

2 participants