Conversation
Reviewer's GuideRefactors the install.sh script to support non-root, jq-free installations with better path detection and macOS Homebrew support, adds Go end-to-end lifecycle tests for pods and serverless resources against live RunPod APIs, and wires them into a GitHub Actions integration-test matrix that exercises both root and non-root modes while cleaning up CI resources. Sequence diagram for CI-triggered e2e integration tests with conditional API key usagesequenceDiagram
actor Dev as Developer
participant GH as GitHub
participant WF as "integration-tests workflow"
participant R as GitHub_Runner
participant GoT as go_test_e2e
participant RP as RunPod_API
Dev->>GH: Push commit / open PR
GH-->>WF: Trigger integration-tests workflow
WF->>R: Start job (matrix: root & non-root)
par Root and non-root matrix
R->>R: Set up environment (install Go, build runpodctl)
R->>R: Evaluate RUNPOD_API_KEY secret
alt RUNPOD_API_KEY present
R->>GoT: go test -tags e2e ./e2e/cli_lifecycle_test.go
GoT->>RP: Create pod / serverless resources
GoT->>RP: Poll for status and exercise lifecycle
GoT->>RP: Delete created resources
GoT-->>R: Exit 0 (tests passed)
else RUNPOD_API_KEY missing (fork or external PR)
R->>GoT: go test -tags e2e ./e2e/cli_lifecycle_test.go
GoT-->>R: Detect missing key and skip e2e tests gracefully
R-->>WF: Mark job as success with skipped tests
end
end
WF-->>GH: Report integration-test status on commit/PR
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The GitHub release version parsing in
install.shusinggrep/sedon the API response is fairly brittle; consider at least validating the extracted string (e.g., it starts withvand contains no whitespace) or falling back with a clearer error when the response shape changes or is rate-limited. - In
e2e/cli_lifecycle_test.go, several filesystem operations (os.WriteFile,os.MkdirAll,os.RemoveAll) ignore returned errors; checking and failing early on these would make test failures easier to diagnose and avoid silent misbehavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The GitHub release version parsing in `install.sh` using `grep`/`sed` on the API response is fairly brittle; consider at least validating the extracted string (e.g., it starts with `v` and contains no whitespace) or falling back with a clearer error when the response shape changes or is rate-limited.
- In `e2e/cli_lifecycle_test.go`, several filesystem operations (`os.WriteFile`, `os.MkdirAll`, `os.RemoveAll`) ignore returned errors; checking and failing early on these would make test failures easier to diagnose and avoid silent misbehavior.
## Individual Comments
### Comment 1
<location path="install.sh" line_range="111" />
<code_context>
local version_url="https://api.github.com/repos/runpod/runpodctl/releases/latest"
- VERSION=$(wget -q -O- "$version_url" | jq -r '.tag_name')
+ # Using grep/sed instead of jq for zero-dependency parsing
+ VERSION=$(wget -q -O- "$version_url" | grep '"tag_name":' | sed -E 's/.*"tag_name": "([^"]+)".*/\1/')
+
if [ -z "$VERSION" ]; then
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The ad‑hoc grep/sed JSON parsing of the GitHub API response is fairly brittle and may break if the response format changes or includes multiple tag_name fields.
This zero-dependency approach increases the risk of extracting the wrong value (e.g., from HTML error pages, rate‑limit responses, or future responses with additional `"tag_name"` fields). To mitigate this, you could either tighten the grep/sed pattern (e.g., match a more specific JSON context or anchor at line start) and/or validate `VERSION` against an expected tag format (e.g., `^v[0-9]`). That helps avoid silently using an incorrect or non-existent release.
Suggested implementation:
```
# Using grep/sed instead of jq for zero-dependency parsing
# - Restrict to the first matching tag_name line
# - Expect the canonical JSON indentation for the field
VERSION=$(wget -q -O- "$version_url" \
| grep -m1 '^ "tag_name":' \
| sed -E 's/^[^"]*"tag_name": "([^"]+)".*/\1/')
```
```
# Ensure we got a plausible semantic version tag (e.g., v1.2.3)
if [ -z "$VERSION" ] || ! [[ "$VERSION" =~ ^v[0-9]+(\.[0-9]+)*$ ]]; then
echo "Failed to fetch a valid latest version of runpodctl (got: '${VERSION:-<empty>}')."
exit 1
```
These changes assume:
1. The script is executed with Bash (for the `[[ ... =~ ... ]]` regex test). If `install.sh` is intended for `/bin/sh`, replace the regex check with a `case` statement instead.
2. The GitHub API continues to indent `"tag_name"` with two spaces. If you want to be more permissive, you can relax the `grep` pattern to `grep -m1 '"tag_name":'` at the cost of slightly looser matching.
</issue_to_address>
### Comment 2
<location path="e2e/cli_lifecycle_test.go" line_range="161-164" />
<code_context>
+ t.Errorf("Pod ID %s not found in list output", podID)
+ }
+
+ // Get Pod
+ t.Logf("Getting pod details...")
+ getOut, getErr := runCLI("pod", "get", podID, "--output", "json")
+ if getErr != nil {
+ t.Errorf("Failed to get pod: %v\nOutput: %s", getErr, getOut)
+ }
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen pod lifecycle assertions by validating the returned data, not just the absence of errors
Right now the test only verifies that `pod get` exits successfully. To better validate the CLI contract, you could parse `getOut` (as with `extractIDField`) and assert that the returned `id` equals `podID` and, after the update, that the `name` equals `newName`. This will help catch regressions in the CLI output shape or contents, not just failures to run the command.
</issue_to_address>
### Comment 3
<location path="e2e/cli_lifecycle_test.go" line_range="328-172" />
<code_context>
+
+ t.Logf("Endpoint is ready and propagated.")
+
+ // List
+ listOut, listErr := runCLI("serverless", "list", "--output", "json")
+ if listErr != nil {
+ t.Errorf("Failed to list endpoints: %v\nOutput: %s", listErr, listOut)
+ } else if !strings.Contains(listOut, epID) {
+ t.Errorf("Endpoint ID %s not found in list output", epID)
+ }
+
+ // Update
+ newName := epName + "-updated"
+ t.Logf("Updating endpoint name to %s...", newName)
+ updateOut, updateErr := runCLI("serverless", "update", epID, "--name", newName)
+ if updateErr != nil {
+ t.Errorf("Failed to update serverless endpoint: %v\nOutput: %s", updateErr, updateOut)
+ }
</code_context>
<issue_to_address>
**suggestion (testing):** Assert serverless endpoint state (ID and updated name) from CLI output to fully validate the lifecycle
Right now this test only checks that `list`/`get` don’t error and that `listOut` contains `epID`, but it never confirms the update took effect. To tighten this:
- Parse `listOut`/`get` as JSON and assert there’s an entry for `epID`.
- After `update`, call `serverless get` and assert the name equals `newName`.
This ensures we verify actual state changes, not just successful exit codes.
Suggested implementation:
```golang
t.Logf("Endpoint is ready and propagated.")
// List endpoints and assert the created endpoint exists
listOut, listErr := runCLI("serverless", "list", "--output", "json")
if listErr != nil {
t.Fatalf("Failed to list endpoints: %v\nOutput: %s", listErr, listOut)
}
type serverlessEndpoint struct {
ID string `json:"id"`
Name string `json:"name"`
}
var endpoints []serverlessEndpoint
if err := json.Unmarshal([]byte(listOut), &endpoints); err != nil {
t.Fatalf("Failed to parse serverless list output as JSON: %v\nOutput: %s", err, listOut)
}
var listedEp *serverlessEndpoint
for i := range endpoints {
if endpoints[i].ID == epID {
listedEp = &endpoints[i]
break
}
}
if listedEp == nil {
t.Fatalf("Endpoint ID %s not found in serverless list output", epID)
}
// Update endpoint name
newName := epName + "-updated"
t.Logf("Updating endpoint name to %s...", newName)
updateOut, updateErr := runCLI("serverless", "update", epID, "--name", newName)
if updateErr != nil {
t.Fatalf("Failed to update serverless endpoint: %v\nOutput: %s", updateErr, updateOut)
}
// Get endpoint and assert the name was updated
getOut, getErr := runCLI("serverless", "get", epID, "--output", "json")
if getErr != nil {
t.Fatalf("Failed to get serverless endpoint: %v\nOutput: %s", getErr, getOut)
}
var updatedEp serverlessEndpoint
if err := json.Unmarshal([]byte(getOut), &updatedEp); err != nil {
t.Fatalf("Failed to parse serverless get output as JSON: %v\nOutput: %s", err, getOut)
}
if updatedEp.ID != epID {
t.Fatalf("Expected endpoint ID %s from get, got %s", epID, updatedEp.ID)
}
if updatedEp.Name != newName {
t.Fatalf("Expected endpoint name %s after update, got %s", newName, updatedEp.Name)
}
return fallback
}
```
1. Ensure `encoding/json` is imported at the top of `e2e/cli_lifecycle_test.go`, for example by updating the import block:
- Add: `encoding/json`
2. If the actual JSON shape from `serverless list/get` differs (e.g., wrapped in an object like `{ "endpoints": [...] }` or uses different field names such as `endpointId` / `endpointName`), adjust the `serverlessEndpoint` struct and unmarshalling logic accordingly:
- Update struct tags (e.g., ``ID string `json:"endpointId"` ``).
- If list is wrapped, unmarshal into an intermediate struct like `struct { Endpoints []serverlessEndpoint `json:"endpoints"` }`.
</issue_to_address>
### Comment 4
<location path="e2e/cli_lifecycle_test.go" line_range="190" />
<code_context>
+ t.Errorf("Failed to start pod: %v\nOutput: %s", startErr, startOut)
+ }
+
+ // Test Croc File Transfer (Send/Receive)
+ t.Logf("Testing croc file transfer...")
+ testFileName := "ci-test-file.txt"
</code_context>
<issue_to_address>
**suggestion (testing):** Clarify expectations and failure behavior for the croc send/receive section to avoid silent test degradation
This croc file transfer block is currently best-effort and hides several failure modes (missing binary, send not starting, code not captured in time, receive failing). That means the main test can still pass while croc support is effectively broken. Please either:
- Make the croc check explicitly optional (e.g., behind a `RUNPOD_E2E_TEST_CROC` env var) and log/skip clearly when disabled, or
- Treat it as required by asserting that `binaryPath` is found and `sendCmd.Start()` (and other critical steps) succeed, failing the test if those expectations aren’t met.
This will surface when croc coverage is actually running in CI instead of silently being skipped.
Suggested implementation:
```golang
// Test Croc File Transfer (Send/Receive)
enableCroc := os.Getenv("RUNPOD_E2E_TEST_CROC") != ""
if !enableCroc {
t.Logf("Skipping croc file transfer test: RUNPOD_E2E_TEST_CROC not set")
} else {
t.Logf("RUNPOD_E2E_TEST_CROC set; croc file transfer test is required")
}
t.Logf("Testing croc file transfer...")
testFileName := "ci-test-file.txt"
testFileContent := "v1.14.15-ci-test"
if err := os.WriteFile(testFileName, []byte(testFileContent), 0644); err != nil && enableCroc {
t.Fatalf("Failed to create croc test file %q: %v", testFileName, err)
}
defer os.Remove(testFileName)
```
To fully implement the behavior described in your review comment, you should:
1. **Guard the entire croc test block with `enableCroc`:**
- Wrap all subsequent croc send/receive logic (starting at `// Test Croc File Transfer (Send/Receive)` and ending where the croc-related code finishes) in:
```go
if enableCroc {
// existing croc send/receive code...
}
```
This ensures nothing runs when `RUNPOD_E2E_TEST_CROC` is not set.
2. **Fail fast when croc is required but the binary is missing:**
- Immediately after the `for _, p := range []string{...}` loop that resolves `binaryPath`, add:
```go
if enableCroc && binaryPath == "" {
t.Fatalf("RUNPOD_E2E_TEST_CROC is set but runpodctl binary was not found in any of the expected paths")
}
```
3. **Treat critical croc steps as required when enabled:**
For the send/receive commands (which are not visible in the snippet), ensure:
- `exec.Command` creation is checked for errors (if applicable; usually it returns *Cmd only).
- `sendCmd.Start()` is checked:
```go
if err := sendCmd.Start(); err != nil && enableCroc {
t.Fatalf("Failed to start croc send command: %v", err)
}
```
- Any subsequent `Wait()`, `Run()`, or `CombinedOutput()` calls for both send and receive commands are checked, and when `enableCroc` is true, failures should call `t.Fatalf` with clear messages (include stderr/stdout in the message).
4. **Imports:**
- Ensure `os` and `os/exec` are already imported at the top of the file. If not, add:
```go
import (
"os"
"os/exec"
// ...other imports...
)
```
The code above assumes these packages are available.
With these changes, croc coverage will be explicitly optional (via `RUNPOD_E2E_TEST_CROC`), clearly logged when disabled, and strictly enforced when enabled, avoiding silent degradation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
|
@sourcery-ai review, please? |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The integration workflow is configured with
go-version: '1.25.7', which does not exist yet and will cause the job to fail; update this to a currently supported Go release (e.g., 1.22.x) that matches the project’s target version. - The croc code extraction logic in
TestE2E_CLILifecycle_Podis quite brittle (scanning arbitrary lines for hyphenated tokens); consider matching the exact expected output format fromrunpodctl sendor printing a machine-readable code line to make the parsing deterministic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The integration workflow is configured with `go-version: '1.25.7'`, which does not exist yet and will cause the job to fail; update this to a currently supported Go release (e.g., 1.22.x) that matches the project’s target version.
- The croc code extraction logic in `TestE2E_CLILifecycle_Pod` is quite brittle (scanning arbitrary lines for hyphenated tokens); consider matching the exact expected output format from `runpodctl send` or printing a machine-readable code line to make the parsing deterministic.
## Individual Comments
### Comment 1
<location path="install.sh" line_range="223-226" />
<code_context>
- echo "runpodctl installed successfully."
-}
+ if ! mv "$cli_file_name" "$INSTALL_DIR/"; then
+ echo "Failed to move $cli_file_name to $INSTALL_DIR/."
+ exit 1
+ fi
</code_context>
<issue_to_address>
**suggestion:** On move failure, the extracted binary is left behind in the working directory.
The archive is cleaned up (`rm -f "$cli_archive_file_name"`), but if `mv` to `$INSTALL_DIR` fails, the extracted `runpodctl` remains in the current directory. Please either delete it on failure (e.g. `rm -f "$cli_file_name"`) or at least print its path so users can handle it explicitly.
```suggestion
if ! mv "$cli_file_name" "$INSTALL_DIR/"; then
echo "Failed to move $cli_file_name to $INSTALL_DIR/."
echo "Removing extracted binary at '$(pwd)/$cli_file_name' to avoid leaving stray files behind."
rm -f "$cli_file_name"
exit 1
fi
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
FNGarvin
force-pushed
the
fng-infra-install-fix-v2
branch
5 times, most recently
from
147f55d to
2a39607
Compare
FNGarvin
force-pushed
the
fng-infra-install-fix-v2
branch
from
2a39607 to
ec2d06f
Compare
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
1 participant
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.
Fng infra install fix v2
Replaces and combines runpod#240 and runpod#241.
Hot off the presses: freshly rebased for latest status of main as of this writing.
Test plan:
Verify that root and non-root installs still work in a clean Linux environment:
podman run --rm -v $(pwd):/workspace alpine:latest sh -c 'apk add --no-cache bash wget grep sed tar coreutils jq curl && cp /workspace/install.sh /tmp/install.sh && chmod 755 /tmp/install.sh && echo "=== TESTING ROOT INSTALL ===" && cd /tmp && bash /tmp/install.sh && /usr/local/bin/runpodctl version && echo -e "\n=== TESTING NON-ROOT INSTALL ===" && adduser -D tester && su tester -c "cd /home/tester && mkdir -p ~/.local/bin && bash /tmp/install.sh" && su tester -c "~/.local/bin/runpodctl version"''Local test of new e2e test against live Runpod services:
export RUNPOD_API_KEY=XXXYYYZZZ && go build -o runpodctl main.go && go test -tags e2e -v ./e2e/cli_lifecycle_test.go from shellCI testing:
Summary by Sourcery
Improve the runpodctl installation script for non-root environments and add end-to-end integration testing against live RunPod services.
New Features:
Enhancements:
CI: