-
Notifications
You must be signed in to change notification settings - Fork 94
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
adding how to run docker with Bacalhau #4846
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe pull request updates the documentation in the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
README.md
(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build / Build Binary (windows, amd64)
- GitHub Check: build / Build Binary (darwin, arm64)
- GitHub Check: build / Build Binary (darwin, amd64)
- GitHub Check: build / Build Binary (linux, armv6)
- GitHub Check: build / Build Binary (linux, armv7)
- GitHub Check: lint / lint
- GitHub Check: build / Build Binary (linux, arm64)
- GitHub Check: build / Build Binary (linux, amd64)
🔇 Additional comments (1)
README.md (1)
91-91
: Verify Docker image repository and available tags.Let's verify the container repository URL and available tags.
Also applies to: 96-96
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check if the Docker image repository exists and list available tags # Check if the repository exists and list tags curl -s -H "Accept: application/vnd.github.v3+json" \ "https://api.github.com/orgs/bacalhau-project/packages/container/bacalhau/versions" | \ jq -r '.[] | select(.metadata.container.tags != null) | .metadata.container.tags[]'Length of output: 290
The GitHub API response format appears different than expected, and the script failed while parsing. It’s best to manually review the raw JSON output to verify that the Bacalhau container repository exists and to inspect the tag format.
Action Required: Manual Verification of Docker Container Tag Details
- The API response did not match the expected structure, causing a jq parsing error.
- Please run the script below and manually verify the JSON output that lists available versions and tag details.
🏁 Script executed:
#!/bin/bash # Re-verify the Docker image repository JSON structure for Bacalhau. curl -s -H "Accept: application/vnd.github.v3+json" "https://api.github.com/orgs/bacalhau-project/packages/container/bacalhau/versions" | jq .Length of output: 338
Container Repository Verification: Manual Authentication Required
The automated API request failed with a 401 error due to missing authentication. This prevents us from programmatically verifying the image tags. Please manually authenticate or inspect the repository via the GitHub Packages UI at bacalhau container repository to verify the existence and tag format of the Docker image.
```bash | ||
docker run ghcr.io/bacalhau-project/bacalhau:latest | ||
``` | ||
|
||
This will start a container with Bacalhau already installed. |
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.
Reading this sounds like it will start bacalhau in a container, but it will just call bacalhau
without any flags or subcommands as that is the default entrypoint. It will just print the help output and exit
The actual use should be:
docker run ghcr.io/bacalhau-project/bacalhau:latest <sub-command>
# checking the bacalhau version
docker run ghcr.io/bacalhau-project/bacalhau:latest version
# listing jobs
docker run ghcr.io/bacalhau-project/bacalhau:latest job list
# listing nodes
docker run ghcr.io/bacalhau-project/bacalhau:latest node list
# running an orchestrator
docker run ghcr.io/bacalhau-project/bacalhau:latest serve --orchestrator
# running a compute node with config
docker run ghcr.io/bacalhau-project/bacalhau:latest serve --compute -c <path>
...
or if you can do docker run -it --entrypoint bash ghcr.io/bacalhau-project/bacalhau:latest
, and then call bacalhau commands internally
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.
So this is a good point - however, in thinking about simplification, there's kind of the following.
What do we think the majority of folks will do with this? And then how do we make it so the default flags are correct?
I THINK in priority order, what we would want to enable would be:
- Run a container as a compute node
- Run a container as a compute node with orchestrator specified (no auth)
- Run a container as a compute node with orchestrator specified (auth)
- Run a container as a compute node with full new config
- Run a container as a CLI container
- Run an orchestrator
So, by this logic, I think the defaults should be to have an entrypoint.sh script, and then execute using the defaults. It should have the following in it:
/etc/bacalhau/config.yaml - a file with the default config we have
/etc/node-info - a file with node-info that are read and put into labels automatically
/entrypoint.sh - a script that loads these items in by default, as well as pings any telemetry if we have it enabled
Then executing 1-6 above looks like:
1. docker run ghcr.io/bacalhau-project/bacalhau:latest # joins the public network? not sure
2. docker run -e BACALHAU_COMPUTE_ORCHESTRATORS=<IP> ghcr.io/bacalhau-project/bacalhau:latest # joins an IP address network
3. docker run -e BACALHAU_COMPUTE_ORCHESTRATORS=<IP> BACALHAU_COMPUTE_AUTH_TOKEN=<TOKEN> ghcr.io/bacalhau-project/bacalhau:latest # access the orchestrator with a token
4. docker run -v $(pwd)/config.yaml:/etc/bacalhau/config.yaml ghcr.io/bacalhau-project/bacalhau:latest
5. docker run ghcr.io/bacalhau-project/bacalhau:latest job list --api-host=<IP>
OR
docker run -e API_HOST=<IP> ghcr.io/bacalhau-project/bacalhau:latest job list
And all of the above could also have a node-info injected with -v $(pwd)/node-info:/etc/node-info
. We could also enable picking up -e BACALHAU_LABEL='[a=1, b=2]'
etc.
Thoughts?
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
README.md
(3 hunks)
🔇 Additional comments (4)
README.md (4)
42-43
: Maintain consistent title capitalization.The title changes improve overall consistency, but consider revising the container section title for better style.
-Getting Started with Bacalhau in 1 minute With Containers +Getting Started with Bacalhau in 1 minute with Containers
87-92
: LGTM!Clear introduction with helpful link to container registry.
101-105
: LGTM!Clear instructions for using Docker-in-Docker container.
110-110
: Fix incorrect Docker image tag format.The Docker image tag contains a syntax error with two
:latest
tags.-docker run -v $(pwd)/config.yaml:/etc/bacalhau/config.yaml ghcr.io/bacalhau-project/bacalhau:latest-dind:latest +docker run -v $(pwd)/config.yaml:/etc/bacalhau/config.yaml ghcr.io/bacalhau-project/bacalhau:latest-dind
To use a container, you can use the following command with Docker: | ||
|
||
```bash | ||
docker run ghcr.io/bacalhau-project/bacalhau:latest | ||
``` | ||
|
||
This will start a container with Bacalhau already installed. |
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.
🛠️ Refactor suggestion
Add examples of common Bacalhau commands with Docker.
The current example is incomplete as it will only print help output and exit. Include examples of common operations to help users get started.
```bash
-docker run ghcr.io/bacalhau-project/bacalhau:latest
+# Check Bacalhau version
+docker run ghcr.io/bacalhau-project/bacalhau:latest version
+
+# List jobs
+docker run ghcr.io/bacalhau-project/bacalhau:latest job list
+
+# Run a compute node
+docker run ghcr.io/bacalhau-project/bacalhau:latest serve --compute
+
+# For interactive use
+docker run -it --entrypoint bash ghcr.io/bacalhau-project/bacalhau:latest
<!-- This is an auto-generated comment by CodeRabbit -->
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/logo/Bacalhau-horizontal-white.svg
is excluded by!**/*.svg
📒 Files selected for processing (1)
README.md
(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build / Build Binary (windows, amd64)
- GitHub Check: build / Build Binary (darwin, arm64)
- GitHub Check: build / Build Binary (darwin, amd64)
- GitHub Check: build / Build Binary (linux, armv6)
- GitHub Check: build / Build Binary (linux, armv7)
- GitHub Check: build / Build Binary (linux, arm64)
- GitHub Check: build / Build Binary (linux, amd64)
- GitHub Check: lint / lint
- GitHub Check: lint / go-lint (ubuntu-latest)
🔇 Additional comments (5)
README.md (5)
42-43
: Maintain consistent title capitalization.The title still has inconsistent capitalization. Consider revising:
-Getting Started with Bacalhau in 1 minute With Containers +Getting Started with Bacalhau in 1 minute with Containers
93-99
: Enhance Docker command examples.The current example will only print help output and exit. Include examples of common operations:
```bash -docker run ghcr.io/bacalhau-project/bacalhau:latest +# Check version +docker run ghcr.io/bacalhau-project/bacalhau:latest version + +# List jobs +docker run ghcr.io/bacalhau-project/bacalhau:latest job list + +# Run a compute node +docker run ghcr.io/bacalhau-project/bacalhau:latest serve --compute--- `109-111`: **Fix incorrect Docker image tag format.** The Docker image tag contains a syntax error with two `:latest` tags: ```diff -docker run -v $(pwd)/config.yaml:/etc/bacalhau/config.yaml ghcr.io/bacalhau-project/bacalhau:latest-dind:latest +docker run -v $(pwd)/config.yaml:/etc/bacalhau/config.yaml ghcr.io/bacalhau-project/bacalhau:latest-dind
115-131
: Replace sensitive information with placeholders in example configuration.The example configuration contains what appears to be real credentials. Replace sensitive information with clear placeholder values:
Orchestrators: - - nats://EXAMPLE-7a02-4083-bf08-bcc2f5fbc025.us1.dev-cloud.expanso.dev:4222 + - nats://<NATS-SERVER-URL>:4222 Auth: - Token: "EXAMPLEaEEFukWVffnf5jb9QkpNnwfiBWEk3475csM7ysudpbFTzYBap5c7sWr6" + Token: "<YOUR-AUTH-TOKEN>"
3-3
: Verify the white logo variant exists.Please ensure that the white variant of the logo exists at the specified path.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if the white logo variant exists test -f ./docs/logo/Bacalhau-horizontal-white.svg && echo "Logo exists" || echo "Logo not found"Length of output: 84
White logo variant verified exists.
The shell script successfully confirmed that the file./docs/logo/Bacalhau-horizontal-white.svg
exists in the repository, so the change in the README is correct.
```yaml | ||
NameProvider: puuid | ||
API: | ||
Port: 1234 | ||
Compute: | ||
Enabled: true | ||
Orchestrators: | ||
- nats://EXAMPLE-7a02-4083-bf08-bcc2f5fbc025.us1.dev-cloud.expanso.dev:4222 | ||
Auth: | ||
Token: "EXAMPLEaEEFukWVffnf5jb9QkpNnwfiBWEk3475csM7ysudpbFTzYBap5c7sWr6" | ||
TLS: | ||
RequireTLS: true | ||
AllowListedLocalPaths: | ||
- /my_data:rw | ||
JobAdmissionControl: | ||
AcceptNetworkedJobs: true | ||
``` |
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.
🧹 Nitpick (assertive)
Add validation for configuration file path.
The example assumes the configuration file exists but doesn't handle the case when it's missing.
Consider adding a validation step in your documentation:
# Validate config file exists
test -f config.yaml || { echo "Error: config.yaml not found"; exit 1; }
# Run container with validated config
docker run -v $(pwd)/config.yaml:/etc/bacalhau/config.yaml ghcr.io/bacalhau-project/bacalhau:latest-dind
Summary by CodeRabbit