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.

Sign up for GitHub

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

Jump to bottom

Create single docker image for rollups node #85

Merged
merged 6 commits into from
Sep 14, 2023
Merged

Create single docker image for rollups node #85

merged 6 commits into from
Sep 14, 2023

Conversation

gligneul
Copy link
Contributor

@gligneul gligneul commented Sep 13, 2023 •
edited
Loading

Close #65

@gligneul gligneul requested a review from a team September 13, 2023 19:24
@gligneul gligneul self-assigned this Sep 13, 2023
@gligneul gligneul changed the title Feature/single image Create single docker image for rollups node Sep 13, 2023
@gligneul gligneul force-pushed the feature/single-image branch 2 times, most recently from 74addca to 661b794 Compare September 13, 2023 20:23
@gligneul gligneul requested a review from tuler September 13, 2023 20:30
@gligneul gligneul force-pushed the feature/single-image branch 5 times, most recently from fd2e8d4 to dac33cc Compare September 13, 2023 23:21
@gligneul gligneul marked this pull request as ready for review September 13, 2023 23:41
endersonmaia
endersonmaia approved these changes Sep 14, 2023
View reviewed changes
Copy link
Contributor

@endersonmaia endersonmaia left a comment

Choose a reason for hiding this comment

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

Nice job!

Do you think it's too much job to keep publishing individual images?

Something like that would be enough, I think.

FROM cartesi/rollups-node as rollups-indexer
ENTRYPOINT [ "/opt/cartesi/bin/cartesi-rollups-indexer"]

This way, the impact on helm-chart and examples would be minimal.

build/Dockerfile Outdated Show resolved Hide resolved
build/Dockerfile Show resolved Hide resolved
@gligneul
Copy link
Contributor Author

Do you think it's too much job to keep publishing individual images?

I'd rather not, it should be easy do adapt the examples (and probably the helmcharts). In fact, I will create a branch in examples to test this PR.

@gligneul gligneul force-pushed the feature/single-image branch 2 times, most recently from 0aa759d to b2d06cb Compare September 14, 2023 13:31
@gligneul gligneul changed the base branch from main to feature/get-onchain-machine-hash September 14, 2023 13:31
torives
torives reviewed Sep 14, 2023
View reviewed changes
.github/workflows/code-quality.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Base automatically changed from feature/get-onchain-machine-hash to main September 14, 2023 14:03
tuler
tuler reviewed Sep 14, 2023
View reviewed changes
.github/workflows/build.yml Outdated Show resolved Hide resolved
build/Dockerfile Outdated Show resolved Hide resolved
GMKrieger
GMKrieger reviewed Sep 14, 2023
View reviewed changes
Copy link
Contributor

@GMKrieger GMKrieger left a comment

Choose a reason for hiding this comment

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

Looking good, just a minor comment

@endersonmaia
Copy link
Contributor

When executing --help I see a message that doesn't seems right.

Configuration for ....

❯ docker run -ti --rm cartesi/rollups-node:devel cartesi-rollups-indexer --help | head -n 3
Configuration for Logs

Usage: cartesi-rollups-indexer [OPTIONS]
❯ docker run -ti --rm cartesi/rollups-node:devel cartesi-rollups-state-server --help | head -n 3
Configuration for state-fold state-server

Usage: cartesi-rollups-state-server [OPTIONS]
❯ docker run -ti --rm cartesi/rollups-node:devel cartesi-rollups-graphql-server --help | head -n 3
Configuration for Logs

Usage: cartesi-rollups-graphql-server [OPTIONS]

@gligneul
refactor!: docker build system
292ff68 
- Removed previous docker images
- Created new docker image with all binaries
- Moved build files to build directory
@fmoura
Copy link
Contributor

fmoura commented Sep 14, 2023

Got some error building it locally

167.6    Compiling grpc-interfaces v1.1.0 (/opt/cartesi/src/rollups-node/offchain/grpc-interfaces)
170.0 error: failed to run custom build command for `grpc-interfaces v1.1.0 (/opt/cartesi/src/rollups-node/offchain/grpc-interfaces)`
170.0 
170.0 Caused by:
170.0   process didn't exit successfully: `/opt/cartesi/src/rollups-node/offchain/target/release/build/grpc-interfaces-a42fbfbf963f313e/build-script-build` (exit status: 1)
170.0   --- stdout
170.0   cargo:rerun-if-changed=./grpc-interfaces/versioning.proto
170.0   cargo:rerun-if-changed=./grpc-interfaces/server-manager.proto
170.0   cargo:rerun-if-changed=./grpc-interfaces
170.0 
170.0   --- stderr
170.0   Error: Custom { kind: Other, error: "protoc failed: Could not make proto path relative: ./grpc-interfaces/versioning.proto: No such file or directory\n" }

@gligneul
Copy link
Contributor Author

When executing --help I see a message that doesn't seems right.

@endersonmaia, this is not related to this PR. I think this was caused by #56.

@gligneul
Copy link
Contributor Author

Got some error building it locally

@fmoura I changed the grpc-interfaces location because it is now only used in one place. You need to download the submodule again with git submodule update --init.

@fmoura
Copy link
Contributor

fmoura commented Sep 14, 2023

When executing --help I see a message that doesn't seems right.

Configuration for ....

❯ docker run -ti --rm cartesi/rollups-node:devel cartesi-rollups-indexer --help | head -n 3
Configuration for Logs

Usage: cartesi-rollups-indexer [OPTIONS]
❯ docker run -ti --rm cartesi/rollups-node:devel cartesi-rollups-state-server --help | head -n 3
Configuration for state-fold state-server

Usage: cartesi-rollups-state-server [OPTIONS]
❯ docker run -ti --rm cartesi/rollups-node:devel cartesi-rollups-graphql-server --help | head -n 3
Configuration for Logs

Usage: cartesi-rollups-graphql-server [OPTIONS]

Created a separate PR for this PR#87

@gligneul
Copy link
Contributor Author

I tested it with examples, and it works as expected. The changes to examples are in the following commit cartesi/rollups-examples@main...feature/rollups-node-image

@gligneul gligneul merged commit 292ff68 into main Sep 14, 2023
5 checks passed
@gligneul gligneul deleted the feature/single-image branch September 14, 2023 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@torives torives torives approved these changes

@fmoura fmoura fmoura approved these changes

@endersonmaia endersonmaia endersonmaia approved these changes

@tuler tuler Awaiting requested review from tuler

@GMKrieger GMKrieger Awaiting requested review from GMKrieger

Assignees

@gligneul gligneul

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create a single node image
6 participants
@gligneul @endersonmaia @fmoura @tuler @torives @GMKrieger