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 #124

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

add docker #124

wants to merge 7 commits into from

Conversation

MarketMadi
Copy link
Contributor

created barebones dockerfile to get node running. changed logging to fail gracefully is terminal is not available (like in a docker container) but logs still saved to debug file and display in docker.

it can easily be run and built.

docker build -t civkit-node .
docker run --rm civkit-node

@ffaex
Copy link

ffaex commented Jan 9, 2024

Hi, could you please remove all files from this PR except for Dockerfile, README.md, src/util.rs as those are not relevant for adding docker. I want to avoid merge conflicts. Best is probably by merging my PR and then rebasing.

Dockerfile looks great. Might expose ports for Websockets, so civkit-cli and civkit-sample can connect.

@ffaex
Copy link

ffaex commented Jan 9, 2024

might replace "cargo build" with "cargo build --release --bin=civkitd"

@MarketMadi
Copy link
Contributor Author

Made those changes. Rebased, and exposed ports. ports can also be exposed with -p but good to include in dockerfile. Feel free to review.

Copy link

@ffaex ffaex left a comment

Choose a reason for hiding this comment

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

looks good, should build if you do those 2 little changes

Dockerfile Outdated
# Copy the binaries from the builder stage
COPY --from=builder /usr/src/civkit-node/target/debug/civkitd /usr/local/bin/civkitd
COPY --from=builder /usr/src/civkit-node/target/debug/civkit-cli /usr/local/bin/civkit-cli
COPY --from=builder /usr/src/civkit-node/target/debug/civkit-sample /usr/local/bin/civkit-sample
Copy link

Choose a reason for hiding this comment

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

path should be to release binary I think.

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 agree. if future users want to use debug in docker, they can change those lines pretty easily

COPY --from=builder /usr/src/civkit-node/target/debug/civkitd /usr/local/bin/civkitd
COPY --from=builder /usr/src/civkit-node/target/debug/civkit-cli /usr/local/bin/civkit-cli
COPY --from=builder /usr/src/civkit-node/target/debug/civkit-sample /usr/local/bin/civkit-sample

Copy link

Choose a reason for hiding this comment

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

we are compiling only the "civkitd" binary as well, so we should only copy that

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 left them thinking they could be useful for debugging / development purposes. i think we should do what you propose, keep it lean.

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