feat: parameterize nakama image #84
Conversation
Your org has enabled the Graphite merge queue for merging into mainAdd the label “graphite/merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Docker
Client->>Docker: buildImages()
Docker-->>Client: Log removal of container
Client->>Docker: removeContainer()
Client->>Docker: pullImages()
Docker-->>Client: Error occurred
Client->>Docker: Log error with context
Client->>Docker: Abort progress bar
Possibly related PRs
Suggested reviewers
🪧 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 (
|
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
common/docker/client_image.go(0 hunks)common/docker/service/nakama.go(4 hunks)
💤 Files with no reviewable changes (1)
- common/docker/client_image.go
🔇 Additional comments (1)
common/docker/service/nakama.go (1)
80-80: LGTM!
The parameterized image is correctly used in the container configuration.
zulkhair
force-pushed
the
daim/parameterize_nakama_image
branch
from
32895b4
to
20e29f5
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
zulkhair
force-pushed
the
daim/parameterize_nakama_image
branch
2 times, most recently
from
dbad8be
to
1b86761
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
common/docker/client_image.go(0 hunks)common/docker/service/evm.go(3 hunks)common/docker/service/nakama.go(4 hunks)
💤 Files with no reviewable changes (1)
- common/docker/client_image.go
🔇 Additional comments (4)
common/docker/service/evm.go (2)
86-86: Service configuration changes look good
The image and platform configurations are properly applied to the service.
Also applies to: 106-106
Line range hint 52-63: Security: Consider removing hardcoded sensitive values
The default values for chainKeyMnemonic, routerKey, and other sensitive configurations are hardcoded. This could lead to accidental use in production environments.
Consider:
- Moving these to a separate development configuration file
- Requiring these values to be explicitly set in production
- Adding validation to ensure development defaults aren't used in production
Let's check if these default values are used elsewhere:
common/docker/service/nakama.go (2)
57-60: LGTM with consideration of previous feedback
The implementation correctly allows for image customization while maintaining backward compatibility.
123-123: LGTM!
The platform configuration is correctly applied to the service struct.
zulkhair
force-pushed
the
daim/parameterize_nakama_image
branch
from
1b86761
to
4f9e89d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (1)
common/docker/service/evm.go (1)
Line range hint
86-106: Review sensitive data handling practicesThe service configuration exposes sensitive data (keys, mnemonics) through environment variables. This is a security anti-pattern as:
- Environment variables are often logged
- They're accessible to all processes running as the same user
- They can persist in shell history
Consider:
- Using a secure secret management service
- Implementing runtime secret injection
- Using temporary environment variables that are unset after reading
Would you like assistance in implementing a more secure configuration pattern?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
common/docker/client_image.go(0 hunks)common/docker/service/evm.go(3 hunks)common/docker/service/nakama.go(4 hunks)
💤 Files with no reviewable changes (1)
- common/docker/client_image.go
🔇 Additional comments (5)
common/docker/service/evm.go (3)
67-70: Consider using a specific version tag instead of 'latest'
This issue was previously identified. Using the 'latest' tag can lead to non-deterministic deployments and unexpected breaks. Consider using a specific version tag as default and documenting the supported version range.
72-81:
Add input validation for platform configuration
- The previous review comment about platform validation remains unaddressed.
- Additionally, accepting unvalidated input from environment variables poses a security risk. Consider:
- Sanitizing input to prevent injection attacks
- Validating against a whitelist of supported OS/ARCH combinations
- Adding proper error handling for malformed input
67-81: Verify image compatibility and platform support
Let's ensure the changes maintain compatibility across services and platforms.
✅ Verification successful
Platform configuration follows consistent pattern across services
The implementation follows the same pattern used in other services (specifically Nakama):
- Both EVM and Nakama services use identical platform configuration logic
- Both allow customization via environment variables (
*_IMAGEand*_IMAGE_PLATFORM) - The platform configuration is properly integrated into the
Servicestruct - The OS/Architecture split logic is consistent across services
The changes maintain compatibility and follow established patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify image compatibility and platform support
# Check if the image exists and supports specified platforms
echo "Checking image availability and platform support..."
rg -g '*.go' -A 5 'ghcr.io/argus-labs/world-engine-evm'
# Check for other services using similar platform configuration
echo "Checking platform configuration patterns..."
rg -g '*.go' 'ocispec.Platform'
# Look for related configuration in other files
echo "Checking related configuration..."
fd -e toml -x cat {} \; | grep -i "EVM_IMAGE\|PLATFORM"
Length of output: 1080
Script:
#!/bin/bash
# Let's analyze the platform configuration usage in more detail
# Check the full context of platform configuration in service files
echo "=== Platform configuration in service files ==="
rg -g '*.go' -B 5 -A 10 'ocispec.Platform'
# Check for any environment variable usage patterns
echo "=== Environment variable patterns ==="
rg -g '*.go' "DockerEnv\[.*PLATFORM"
# Check for any image configuration patterns
echo "=== Image configuration patterns ==="
rg -g '*.go' "DockerEnv\[.*IMAGE"
# Check service configuration structure
echo "=== Service configuration structure ==="
ast-grep --pattern 'type $_ struct {
$$$
Platform $$$
$$$
}'
Length of output: 5249
common/docker/service/nakama.go (2)
57-60: Consider using a specific version tag instead of 'latest'
While the implementation correctly allows image parameterization, using 'latest' as the default tag can lead to non-deterministic deployments. Consider:
- Setting a specific version as default
- Adding a constant for the default image
+const defaultNakamaImage = "ghcr.io/argus-labs/world-engine-nakama:v1.0.0" // Update version as needed
func Nakama(cfg *config.Config) Service {
// ...
- nakamaImage := "ghcr.io/argus-labs/world-engine-nakama:latest"
+ nakamaImage := defaultNakamaImage
if cfg.DockerEnv["NAKAMA_IMAGE"] != "" {
nakamaImage = cfg.DockerEnv["NAKAMA_IMAGE"]
}88-88: LGTM: Service configuration updates look good
The changes to use the parameterized image and platform configuration are implemented correctly.
Also applies to: 123-123
zulkhair
force-pushed
the
daim/parameterize_nakama_image
branch
from
4f9e89d
to
0148219
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range comments (1)
common/docker/service/evm.go (1)
Platform configuration needs to be added for celestia service
The verification revealed inconsistent implementation of platform configuration across services:
- Both EVM and Nakama services have image parameterization via environment variables
- Only EVM service has platform configuration support
- Celestia service uses a ghcr.io image but lacks both image parameterization and platform configuration
🔗 Analysis chain
Line range hint
67-106: Verify consistent implementation across servicesLet's verify that the image and platform parameterization is implemented consistently across other services.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent implementation of image and platform parameterization # Check for image parameterization pattern echo "Checking image parameterization pattern..." rg -U 'Image.*=.*ghcr\.io.*latest.*\n.*if.*\["[A-Z_]+_IMAGE"\].*\n.*=.*\n.*}' common/docker/service/ # Check for platform configuration pattern echo "Checking platform configuration pattern..." rg -U 'var platform ocispec\.Platform.*\n.*if.*\["[A-Z_]+_IMAGE_PLATFORM"\]' common/docker/service/Length of output: 1070
Script:
#!/bin/bash # Let's check if there are other service files that might need similar platform configuration # First, list all service files echo "All service files:" fd "\.go$" common/docker/service/ # Then check for Platform field usage in service structs echo -e "\nPlatform field usage in Service structs:" rg -U "Service\s*{\s*(?s:.)*Platform:\s*\w+" common/docker/service/ # Finally check for any other image configurations that might need platform support echo -e "\nOther image configurations:" rg "Image.*:.*ghcr\.io" common/docker/service/Length of output: 1076
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
common/docker/client_image.go(0 hunks)common/docker/service/evm.go(3 hunks)common/docker/service/nakama.go(4 hunks)common/editor/editor.go(1 hunks)
💤 Files with no reviewable changes (1)
- common/docker/client_image.go
🔇 Additional comments (4)
common/docker/service/evm.go (2)
67-70: Consider using a specific version tag instead of 'latest'
86-86: LGTM: Service configuration updates look good
The changes correctly integrate the parameterized image and platform configuration into the service struct.
Also applies to: 106-106
common/docker/service/nakama.go (1)
88-88: LGTM: Service configuration updates are correct
The dynamic image reference and platform configuration are properly integrated into the service struct.
Also applies to: 123-123
common/editor/editor.go (1)
153-156: Verify PR scope: This change seems unrelated to nakama image parameterization.
While this is a good improvement to error handling, it appears unrelated to the PR's stated objective of parameterizing the nakama image. Consider splitting unrelated changes into separate PRs to maintain clear change scope and simplify review.
zulkhair
force-pushed
the
daim/parameterize_nakama_image
branch
from
0148219
to
30ae85f
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range comments (1)
common/docker/service/evm.go (1)
Line range hint
41-65: Consider moving sensitive configuration to a secure storeThe code contains hardcoded sensitive values (router keys, mnemonics). Consider:
- Moving these to a secure configuration management system
- Using environment variables with secure defaults
- Adding documentation about secure configuration practices
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
cmd/world/root/root_test.go(2 hunks)common/docker/client_image.go(0 hunks)common/docker/service/evm.go(3 hunks)common/docker/service/nakama.go(4 hunks)common/editor/editor.go(1 hunks)
💤 Files with no reviewable changes (1)
- common/docker/client_image.go
🔇 Additional comments (3)
common/docker/service/evm.go (1)
86-86: LGTM: Service configuration changes look good
The integration of the configurable image and platform settings into the Service struct is clean and aligns well with the PR objectives.
Also applies to: 106-106
common/docker/service/nakama.go (2)
6-6: LGTM: Import addition is appropriate
The strings package import is required for the new platform string parsing functionality.
88-88: LGTM: Service configuration updates are correct
The changes properly integrate the parameterized image and platform configuration into the service struct.
Also applies to: 123-123
Closes: WORLD-XXX ## Overview Change nakama image to alway use latest version and can be parameterize through world.toml if user want to use specific version ## Brief Changelog - Change nakama default image to latest version - Add `NAKAMA_IMAGE` and `NAKAMA_IMAGE_PLATFORM` on world.toml to use specific version of nakama ## Testing and Verifying Tested manually using `world cardinal start` command <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced configurability for the Nakama container image and platform settings via environment variables. - Improved configurability for the EVM container image and platform settings based on environment variables. - **Improvements** - Improved logging for container removal during image building. - Enhanced error reporting and progress handling when pulling images. - Added robust error handling for asset availability in the release download process. - Increased retry duration for service status checks, allowing more time for services to become available. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
zulkhair
force-pushed
the
daim/parameterize_nakama_image
branch
from
30ae85f
to
caa3fc3
Compare
Merge activity
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #84 +/- ##
==========================================
- Coverage 46.84% 46.60% -0.25%
==========================================
Files 56 56
Lines 3546 3575 +29
==========================================
+ Hits 1661 1666 +5
- Misses 1675 1694 +19
- Partials 210 215 +5 ☔ View full report in Codecov by Sentry. |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Closes: WORLD-XXX
Overview
Change nakama image to alway use latest version and can be parameterize through world.toml if user want to use specific version
Brief Changelog
NAKAMA_IMAGEandNAKAMA_IMAGE_PLATFORMon world.toml to use specific version of nakamaTesting and Verifying
Tested manually using
world cardinal startcommandSummary by CodeRabbit