-
Notifications
You must be signed in to change notification settings - Fork 255
fix(cmd/testnetify): pass config by pointer #2018
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
WalkthroughChange: switch TestnetConfig from value to pointer in InitAkashAppForTestnet and its caller; add nil check. Shell script: change process cleanup to per-PID loop and replace Changes
Sequence Diagram(s)(omitted — changes are type/signature adjustments and script edits; no new runtime control flow to visualize) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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
🧹 Nitpick comments (1)
script/upgrades.sh (1)
172-176: Remove unnecessary shellcheck disable directive.The shellcheck disable comment on line 173 is no longer needed since
$pidis now properly quoted by thereadcommand and not subject to word splitting. The refactored loop correctly handles PIDs without unquoted expansion.Apply this diff to remove the obsolete directive:
function cleanup() { while IFS= read -r pid; do - # shellcheck disable=SC2086 kill $pid done < <(jobs -p) }Alternatively, you could explicitly quote
$pidfor consistency:function cleanup() { while IFS= read -r pid; do - # shellcheck disable=SC2086 - kill $pid + kill "$pid" done < <(jobs -p) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/testnet.go(1 hunks)cmd/akash/cmd/app_creator.go(1 hunks)script/upgrades.sh(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/testnet.go (1)
cmd/akash/cmd/testnetify/config.go (1)
TestnetConfig(61-67)
cmd/akash/cmd/app_creator.go (2)
app/testnet.go (1)
TestnetConfig(59-64)cmd/akash/cmd/testnetify/config.go (1)
TestnetConfig(61-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: network-upgrade
- GitHub Check: lint-go
- GitHub Check: coverage
- GitHub Check: release-dry-run
- GitHub Check: build-bins
- GitHub Check: build-macos
- GitHub Check: sims
- GitHub Check: tests
🔇 Additional comments (1)
script/upgrades.sh (1)
366-366: Clarify unrelated CLI change and resolve codebase inconsistency.The command changed from
$AKASH initto$AKASH genesis init, which appears unrelated to the PR objective ("pass config by pointer"). Additionally, this change creates an inconsistency:_build/single-node.sh:18still uses the oldakash initpattern and was not modified.Confirm:
- Is this a required Akash CLI version change?
- Should
_build/single-node.shalso be updated for consistency?- If CLI-related, this should be documented or handled in a separate PR aligned with the actual scope of "passing config by pointer."
Signed-off-by: Artur Troian <[email protected]>
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
🧹 Nitpick comments (1)
script/upgrades.sh (1)
172-175: Consider adding error handling for process cleanup.The per-PID loop approach is valid, but the cleanup could be more robust. Currently, if
killfails on a PID (e.g., process already terminated), the script continues without feedback.Consider this enhanced version with error handling and explicit signal:
function cleanup() { - while IFS= read -r pid; do - # shellcheck disable=SC2086 - kill $pid - done < <(jobs -p) + while IFS= read -r pid; do + if kill -0 "$pid" 2>/dev/null; then + kill -TERM "$pid" 2>/dev/null || true + fi + done < <(jobs -p) }This checks if the PID exists before attempting to kill it and explicitly uses SIGTERM.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/testnet.go(1 hunks)cmd/akash/cmd/app_creator.go(1 hunks)script/upgrades.sh(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/akash/cmd/app_creator.go
🧰 Additional context used
🧬 Code graph analysis (1)
app/testnet.go (2)
cmd/akash/cmd/testnetify/config.go (1)
TestnetConfig(61-67)app/app.go (1)
AkashApp(92-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: tests
- GitHub Check: sims
- GitHub Check: lint-go
- GitHub Check: release-dry-run
- GitHub Check: build-bins
- GitHub Check: coverage
- GitHub Check: build-macos
🔇 Additional comments (2)
script/upgrades.sh (1)
366-366: Command structure update looks correct.The change from
$AKASH initto$AKASH genesis initaligns with a command restructuring whereinitbecame a subcommand undergenesis. This is consistent with the PR objectives.app/testnet.go (1)
72-80: LGTM! Nil check properly implemented.The parameter type change to
*TestnetConfigand the nil safety check address the concern raised in the previous review. The use oftmos.Exitis consistent with the existing error handling pattern in this function (line 86), and all subsequent dereferences oftcfg(lines 135, 146, 259, 260, 271, 290, 292) are safe.
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...
!to the type prefix if API or client breaking changeCHANGELOG.md