Merged
Conversation
- Set the SHELL variable to /bin/bash in the Makefile for improved script compatibility.
- Updated GetMinitiadBinaryPath to include OS and architecture in the binary path for Linux, improving compatibility. - Refactored NewMinitiadQuerier and downloadMinitiaApp to utilize the updated binary path resolution, ensuring correct binary retrieval based on the environment.
- Consolidated the binary path resolution logic in GetMinitiadBinaryPath to handle both Darwin and Linux operating systems uniformly, enhancing code clarity and maintainability.
- Updated the permission setting for binaries in InstallMinitiadBinary and NewMinitiadQuerier to use a more concise error handling approach. - Cleaned up whitespace in RecoverKeyFromMnemonicWithCoinType for improved readability. - Ensured consistent permission setting logic across multiple files, enhancing code clarity and maintainability.
- Eliminated the SHELL variable assignment in the Makefile as it is no longer needed, streamlining the build configuration.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCentralized minitiad binary resolution, added FindBinaryDir to locate executables, and unified permission setting to run unconditionally. Callers updated to use the helper. Added Systemd helpers to detect/enable lingering with a sudo fallback and adjusted service launch/upgrade paths. Minor formatting tweak in key recovery code. Changes
Sequence Diagram(s)sequenceDiagram
participant Launcher as Launcher
participant Systemd as Systemd helper
participant OS as OS (file system)
participant Loginctl as loginctl / sudo
Launcher->>Systemd: ensureUserServicePrerequisites()
Systemd->>OS: isLingeringEnabled() (check /var/lib/systemd/linger/<user>)
alt lingering enabled
Systemd-->>Launcher: skip enablement
else lingering not enabled
Systemd->>Loginctl: run "loginctl enable-linger <user>"
alt success
Loginctl-->>Systemd: success
Systemd-->>Launcher: enabled
else failure
Systemd->>Loginctl: run "sudo loginctl enable-linger <user>"
alt sudo success
Loginctl-->>Systemd: success (sudo)
Systemd-->>Launcher: enabled
else sudo failure
Loginctl-->>Systemd: error output
Systemd-->>Launcher: return formatted error with sudo output
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
- Introduced a new method to check if systemd lingering is enabled for the user by inspecting the linger state file directly, avoiding potential permission issues with loginctl. - Updated the ensureUserServicePrerequisites method to utilize the new check, improving error handling and user guidance when enabling lingering.
- Added a new method to enable systemd lingering for the current user, attempting both loginctl and sudo for improved compatibility on cloud VMs. - Updated ensureUserServicePrerequisites to utilize the new enableLingering method, enhancing error handling and user guidance.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cosmosutils/cli_query.go (1)
76-104: ReuseInstallMinitiadBinaryhere instead of carrying another copy of the installer flow.This block is still a second copy of the download/extract/chmod sequence from
cosmosutils.InstallMinitiadBinary, and the same manual sequence exists again inmodels/minitia/launch.go:2165-2199. KeepingNewMinitiadQuerieron the helper avoids having to replicate the next archive-layout fix in multiple places.Proposed simplification
- userHome, err := os.UserHomeDir() - if err != nil { - return nil, fmt.Errorf("failed to get user home dir: %v", err) - } binaryPath, err := GetMinitiadBinaryPath(DefaultMinitiadQuerierVM, version) if err != nil { return nil, fmt.Errorf("failed to get binary path: %v", err) } - weaveDataPath := filepath.Join(userHome, common.WeaveDataDirectory) - tarballPath := filepath.Join(weaveDataPath, "minitia.tar.gz") - extractedPath := filepath.Join(weaveDataPath, fmt.Sprintf("mini%s@%s", DefaultMinitiadQuerierVM, version)) - - if _, err := os.Stat(binaryPath); os.IsNotExist(err) { - if _, err := os.Stat(extractedPath); os.IsNotExist(err) { - err := os.MkdirAll(extractedPath, os.ModePerm) - if err != nil { - return nil, fmt.Errorf("failed to create weave data directory: %v", err) - } - } - - if err = io.DownloadAndExtractTarGz(downloadURL, tarballPath, extractedPath); err != nil { - return nil, fmt.Errorf("failed to download minitia binary: %v", err) - } - } - - if err := os.Chmod(binaryPath, 0755); err != nil { - return nil, fmt.Errorf("failed to set permissions for binary: %v", err) + if err := InstallMinitiadBinary(DefaultMinitiadQuerierVM, version, downloadURL, binaryPath); err != nil { + return nil, fmt.Errorf("failed to install minitiad binary: %v", err) }Then remove the now-unused imports in this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cosmosutils/cli_query.go` around lines 76 - 104, The NewMinitiadQuerier factory duplicates the download/extract/chmod flow already implemented in InstallMinitiadBinary; replace the manual sequence (calls around GetMinitiadBinaryPath, os.Stat, io.DownloadAndExtractTarGz, os.MkdirAll and os.Chmod) by invoking InstallMinitiadBinary(DefaultMinitiadQuerierVM, version, downloadURL) (or the appropriate InstallMinitiadBinary signature) and then continue using GetMinitiadBinaryPath as before; after switching to InstallMinitiadBinary, remove the now-unused imports introduced for the manual flow and any local helper variables that become redundant (e.g., tarballPath, extractedPath, weaveDataPath).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cosmosutils/cli_query.go`:
- Around line 76-104: The NewMinitiadQuerier factory duplicates the
download/extract/chmod flow already implemented in InstallMinitiadBinary;
replace the manual sequence (calls around GetMinitiadBinaryPath, os.Stat,
io.DownloadAndExtractTarGz, os.MkdirAll and os.Chmod) by invoking
InstallMinitiadBinary(DefaultMinitiadQuerierVM, version, downloadURL) (or the
appropriate InstallMinitiadBinary signature) and then continue using
GetMinitiadBinaryPath as before; after switching to InstallMinitiadBinary,
remove the now-unused imports introduced for the manual flow and any local
helper variables that become redundant (e.g., tarballPath, extractedPath,
weaveDataPath).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 69c5f4e6-7ec6-46bf-a232-1e5f23e620b2
📒 Files selected for processing (4)
cosmosutils/binary.gocosmosutils/cli_query.gocosmosutils/keys.gomodels/minitia/launch.go
- Modified the binary path resolution logic in the Create method to include Minitia in the switch case, ensuring correct path handling for this command. - Removed redundant case handling for Minitia, streamlining the code and improving maintainability.
- Implemented FindBinaryDir function to dynamically locate the binary directory for Minitia, improving flexibility and compatibility with varying tarball structures. - Updated the Create methods in both systemd and launchd services to utilize the new binary path resolution logic, ensuring accurate binary retrieval for Minitia commands. - Enhanced error handling for binary location failures, providing clearer feedback on issues during the binary extraction process.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/upgrade.go`:
- Around line 124-128: FindBinaryDir(extractedPath, "weave") can match an
existing stale weave under the shared data root and cause deletion of the wrong
file; change the workflow so the release/archive is extracted into a
uniquely-named temporary directory (not the shared data root) before calling
FindBinaryDir, then use the resulting binaryDir/binaryPath from that temp
extraction and perform cleanup on that temp directory only; update any
references to extractedPath passed into FindBinaryDir to point to the temp
extraction dir and ensure deferred removal targets the temp dir containing the
newly extracted assets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4db7ea67-917e-4f88-9828-f413e42ca05d
📒 Files selected for processing (4)
cmd/upgrade.gocosmosutils/binary.goservice/launchd.goservice/systemd.go
✅ Files skipped from review due to trivial changes (1)
- service/systemd.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cosmosutils/binary.go
- Changed the binary extraction process to utilize a temporary directory, preventing conflicts with existing binaries in the shared data root. - Updated the download and extraction logic to reflect the new temporary path, enhancing reliability during upgrades. - Improved error handling for directory creation and binary location, ensuring clearer feedback on potential issues.
- Replaced GetMinitiadBinaryPath and InstallMinitiadBinary with a new EnsureMinitiadBinary function to streamline binary retrieval and installation. - Improved error handling and clarity in the binary extraction process, ensuring the binary is correctly located or downloaded as needed. - Updated related functions across the codebase to utilize the new binary management approach, enhancing maintainability and reducing redundancy.
restorenode
requested changes
Mar 19, 2026
- Introduced new test cases for FindBinaryDir to validate binary location logic across various directory structures and conditions. - Added tests for EnsureMinitiadBinary to ensure correct binary retrieval from nested directories, enhancing coverage and reliability of binary management functions. - Implemented helper function createExecutable to streamline test setup for executable files.
songwongtp
approved these changes
Mar 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
Refactor
New Features
New Behavior