-
Notifications
You must be signed in to change notification settings - Fork 58
fix: docker-compose tutorial #581
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
Conversation
WalkthroughThe tutorial and associated Docker configuration were updated to remove the Sequencer node, focusing solely on running the DA node and the gm-world chain. Dockerfile and Docker Compose configurations were revised, with the gm-world Dockerfile renamed and streamlined, and service definitions and build instructions adjusted accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Docker Compose
participant local-da Container
participant gm-world Container
User->>Docker Compose: Run docker-compose up
Docker Compose->>local-da Container: Start DA node
Docker Compose->>gm-world Container: Start gm-world chain with updated entrypoint
gm-world Container->>local-da Container: Interact with DA node (no sequencer involved)
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @pthmas, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request updates the Docker-Compose tutorial in the documentation to reflect the current architecture of the associated 'test-app'. The changes primarily involve removing the local-sequencer
component from the setup, streamlining the tutorial to focus on the gm-world
chain and a local-da
node. This ensures the tutorial provides accurate and up-to-date instructions for building and running the application with Docker Compose.
Highlights
- Tutorial Simplification: The Docker-Compose tutorial has been streamlined by removing all references and configurations related to the
local-sequencer
node, focusing solely on integrating thegm-world
chain with alocal-da
(Data Availability) node. - Updated Dockerfile for
gm-world
: TheDockerfile
for thegm-world
chain has been updated. It now uses the nameDockerfile.gm
, incorporates newignite
commands for building (ignite app install
), and reflects the change in the main executable fromrollkit
togmd
. - Local DA Image Building: The tutorial now guides users to build the
local-da
Docker image locally from source (Dockerfile.da
) rather than relying on a pre-built remote image, making the setup more self-contained and reproducible. - Docker Compose Configuration Update: The
docker-compose.yml
file has been revised to remove thelocal-sequencer
service, adjust thegm-world
service's command arguments to match the newgmd
executable and its flags, and reference the locally builtlocal-da
image.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the Docker Compose tutorial by removing the local-sequencer
and updating various commands to align with the latest practices for the test application. The changes are logical, but I've found a few issues that would prevent a user from successfully completing the tutorial. These include a missing step to clone a required repository and a YAML syntax error in the docker-compose.yml
snippet. I've also included suggestions to improve the tutorial's long-term stability and readability.
437134a
to
35cb1c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tutorials/docker-compose.md (1)
82-99
: Copy path forgmd
binary is wrong – image will fail at runtime
ignite chain build
stores the artefact under./build
(or$WORKDIR/build
) – not in/go/bin/
.
COPY --from=base /go/bin/gmd /usr/bin
therefore produces “no such file” and the final image will not start.-# Build the chain -RUN ignite app install -g github.com/ignite/apps/rollkit -RUN ignite chain build -y -RUN ignite rollkit init +RUN ignite app install -g github.com/ignite/apps/rollkit \ + && ignite chain build -o /usr/local/bin/gmd -y \ + && ignite rollkit init ... -# Copy over the rollkit binary from the build stage -COPY --from=base /go/bin/gmd /usr/bin +# Copy the binary produced above +COPY --from=base /usr/local/bin/gmd /usr/local/bin/gmdPlease adjust or the tutorial will break for newcomers.
♻️ Duplicate comments (1)
tutorials/docker-compose.md (1)
65-69
: Pin versions & avoid curl-|bash for reproducible builds
curl | bash
directly pulls a moving target from the Internet each time the image is built.
That creates (1) non-deterministic images and (2) an unnecessary supply-chain risk.Diff sketch:
-# Install rollkit -RUN curl -sSL https://rollkit.dev/install.sh | bash -# Install ignite -RUN curl https://get.ignite.com/cli! | bash +# Install rollkit & ignite deterministically +ARG ROLLKIT_VERSION=v1.2.3 +ARG IGNITE_VERSION=v0.34.1 + +RUN curl -sSL https://github.com/rollkit/rollkit/releases/download/${ROLLKIT_VERSION}/rollkit_${ROLLKIT_VERSION}_linux_amd64.tar.gz \ + | tar -xz -C /usr/local/bin && \ + curl -sSL https://github.com/ignite/cli/releases/download/${IGNITE_VERSION}/ignite_${IGNITE_VERSION}_linux_amd64.tar.gz \ + | tar -xz -C /usr/local/binThis guarantees repeatable results and lets readers override versions explicitly.
🧹 Nitpick comments (2)
tutorials/docker-compose.md (2)
151-165
: Mixed networking modes may confuse readers
gm-world
usesnetwork_mode: host
whilelocal-da
stays on the default bridge network.
Connectivity works only because port 7980 is published on the host, but:
- Host networking disables Compose’s DNS – you cannot reference
local-da:7980
.- The example sacrifices isolation and won’t work on macOS/Windows.
Consider keeping both services on the default network:
- network_mode: host + networks: + - default ... - "--rollkit.da.address", - "http://0.0.0.0:7980", + "--rollkit.da.address", + "http://local-da:7980",This is portable and easier to reason about.
168-172
: YAML comment indentation – minor cleanupThe comment under
image: local-da
is indented two extra spaces.
While YAML ignores comments, aligning it with neighbouring keys improves readability and avoids false-positive lints.- # Set the name of the docker container for ease of use + # Set the name of the docker container for ease of use
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tutorials/docker-compose.md
(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: assafmo
PR: rollkit/docs#390
File: guides/assets/hyperlane-use-tia-for-gas/wasmd.Dockerfile:5-6
Timestamp: 2024-10-11T19:05:20.591Z
Learning: In `guides/assets/hyperlane-use-tia-for-gas/wasmd.Dockerfile`, the project chooses not to use a `.dockerignore` file during Docker builds.
Learnt from: assafmo
PR: rollkit/docs#390
File: guides/assets/hyperlane-use-tia-for-gas/wasmd.Dockerfile:5-6
Timestamp: 2024-10-08T09:18:01.381Z
Learning: In `guides/assets/hyperlane-use-tia-for-gas/wasmd.Dockerfile`, the project chooses not to use a `.dockerignore` file during Docker builds.
tutorials/docker-compose.md (2)
Learnt from: assafmo
PR: rollkit/docs#390
File: guides/assets/hyperlane-use-tia-for-gas/wasmd.Dockerfile:5-6
Timestamp: 2024-10-11T19:05:20.591Z
Learning: In `guides/assets/hyperlane-use-tia-for-gas/wasmd.Dockerfile`, the project chooses not to use a `.dockerignore` file during Docker builds.
Learnt from: assafmo
PR: rollkit/docs#390
File: guides/assets/hyperlane-use-tia-for-gas/wasmd.Dockerfile:5-6
Timestamp: 2024-10-08T09:18:01.381Z
Learning: In `guides/assets/hyperlane-use-tia-for-gas/wasmd.Dockerfile`, the project chooses not to use a `.dockerignore` file during Docker builds.
🔇 Additional comments (1)
tutorials/docker-compose.md (1)
108-109
: Validate CLI flag spellingThe flag was renamed in Rollkit 0.12. Make sure the version you pinned (see previous comment) actually supports
--rollkit.node.aggregator
; older binaries expect--rollkit.aggregator
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice job
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tutorials/docker-compose.md (1)
65-69
: Unpinned installs via curl | bash (previous feedback)The tutorial installs Rollkit and Ignite with
curl | bash
without pinning versions, which hurts reproducibility. This concern was raised earlier; the same reasoning applies here.
🧹 Nitpick comments (1)
tutorials/docker-compose.md (1)
169-171
: YAML style nit – comment indentationThe comment appears one indentation level deeper than surrounding keys:
image: local-da # Set the name … container_name: local-daAlthough YAML parsers ignore comment placement, aligning the comment improves readability and avoids confusion:
image: local-da - # Set the name of the docker container for ease of use + # Set the name of the docker container for ease of use container_name: local-da
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tutorials/docker-compose.md
(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: assafmo
PR: rollkit/docs#390
File: guides/assets/hyperlane-use-tia-for-gas/wasmd.Dockerfile:5-6
Timestamp: 2024-10-11T19:05:20.591Z
Learning: In `guides/assets/hyperlane-use-tia-for-gas/wasmd.Dockerfile`, the project chooses not to use a `.dockerignore` file during Docker builds.
Learnt from: assafmo
PR: rollkit/docs#390
File: guides/assets/hyperlane-use-tia-for-gas/wasmd.Dockerfile:5-6
Timestamp: 2024-10-08T09:18:01.381Z
Learning: In `guides/assets/hyperlane-use-tia-for-gas/wasmd.Dockerfile`, the project chooses not to use a `.dockerignore` file during Docker builds.
tutorials/docker-compose.md (2)
Learnt from: assafmo
PR: rollkit/docs#390
File: guides/assets/hyperlane-use-tia-for-gas/wasmd.Dockerfile:5-6
Timestamp: 2024-10-11T19:05:20.591Z
Learning: In `guides/assets/hyperlane-use-tia-for-gas/wasmd.Dockerfile`, the project chooses not to use a `.dockerignore` file during Docker builds.
Learnt from: assafmo
PR: rollkit/docs#390
File: guides/assets/hyperlane-use-tia-for-gas/wasmd.Dockerfile:5-6
Timestamp: 2024-10-08T09:18:01.381Z
Learning: In `guides/assets/hyperlane-use-tia-for-gas/wasmd.Dockerfile`, the project chooses not to use a `.dockerignore` file during Docker builds.
🔇 Additional comments (1)
tutorials/docker-compose.md (1)
98-99
: Verify binary path before copy
COPY --from=base /go/bin/gmd /usr/bin
assumesignite chain build
places thegmd
binary in$GOBIN
(/go/bin
).
On recent Ignite versions the binary is emitted to./build/
instead, so this copy step can silently fail, leaving nogmd
in the runtime image.Either:
- Replace with the actual output path, e.g.
COPY --from=base /app/build/gmd /usr/bin/
- Or add
go install ./...
so the binary lands in/go/bin
.Please confirm the path for the version of Ignite you are using.
Overview
Updated the Docker-Compose tutorial from the documentation with the actual test-app.
Summary by CodeRabbit