-
Notifications
You must be signed in to change notification settings - Fork 18
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
Ubuntu systemctl support & Fix relayer l2 flow #10
Conversation
WalkthroughThe changes in this pull request involve modifications to several files within the Changes
Possibly related PRs
🪧 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: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
cosmosutils/binary.go
(1 hunks)cosmosutils/cli_query.go
(1 hunks)models/relayer/init.go
(2 hunks)service/systemd.go
(2 hunks)
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: CHILL
📒 Files selected for processing (1)
service/systemd.go
(2 hunks)
🔇 Additional comments (3)
service/systemd.go (3)
13-13
: LGTM!
The new import is correctly placed and necessary for the GetInitiaBinaryPath
function used in the changes below.
56-56
:
Initialize serviceName consistently across all command types
The previous review comment about serviceName initialization is still applicable. The template formatting uses command name directly without proper initialization across all cases.
var binaryPath string
+var serviceName string
+serviceName = binaryName // Default to binaryName if not set in switch
switch j.commandName {
case Initia:
binaryPath = filepath.Dir(cosmosutils.GetInitiaBinaryPath(binaryVersion))
case Minitia:
binaryPath = filepath.Join(userHome, common.WeaveDataDirectory, binaryVersion, strings.ReplaceAll(binaryVersion, "@", "_"))
case OPinitExecutor:
serviceName = "executor"
case OPinitChallenger:
serviceName = "challenger"
}
-cmd.Stdin = strings.NewReader(fmt.Sprintf(string(template), binaryName, currentUser.Username, binaryPath, string(j.commandName), appHome))
+cmd.Stdin = strings.NewReader(fmt.Sprintf(string(template), binaryName, currentUser.Username, binaryPath, serviceName, appHome))
44-52
: Verify binary path construction across different environments
The switch statement handles path construction differently for each command type. Please ensure that:
- The paths are correctly constructed on different Ubuntu versions
- The paths exist and are accessible
- The version string replacement in Minitia case handles all possible version formats
✅ Verification successful
Let me gather more information about the binary path construction by checking the related utility functions and path handling logic.
Based on the analysis of the shell script results, I can now provide a final response:
Binary path construction is correctly implemented and consistent
The verification shows that:
- The path construction is consistent with the codebase's established patterns:
- Initia: Uses
GetInitiaBinaryPath()
which handles version formats and OS-specific paths - Minitia: Follows the same pattern as other binaries in the codebase, storing under
WeaveDataDirectory
- Default: Uses the base
WeaveDataDirectory
as expected
- Initia: Uses
- Version string handling is properly implemented:
@
character in versions is handled consistently across the codebase- Version string replacement is used in similar patterns elsewhere
- All paths are constructed relative to the user's home directory using
WeaveDataDirectory
constant
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify binary path construction and permissions
# Test: Check if the paths exist and are accessible
echo "Checking common paths..."
fd -t d "weave" $HOME
# Test: Check version string handling
echo "Checking version string handling..."
rg -A 2 'binaryVersion.*=|version.*='
Length of output: 4738
Script:
#!/bin/bash
# Check the implementation of GetInitiaBinaryPath and related functions
echo "Checking Initia binary path implementation..."
ast-grep --pattern 'func GetInitiaBinaryPath($$$)'
echo "\nChecking WeaveDataDirectory constant..."
rg "WeaveDataDirectory.*=" -A 2
echo "\nChecking related filepath operations..."
rg "filepath\.(Join|Dir)" -A 2
echo "\nChecking version string handling in path construction..."
ast-grep --pattern 'strings.ReplaceAll($version, "@", $_)'
Length of output: 29336
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.
Changes
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation