feat(rayapp): Anyscale CLI pr1 core infrastructure#396
feat(rayapp): Anyscale CLI pr1 core infrastructure#396elliot-barn wants to merge 6 commits intomainfrom
Conversation
Summary of ChangesHello @elliot-barn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes the foundational components for integrating with the Anyscale CLI within the rayapp package. It provides a structured way to check for the CLI's presence, execute commands, and manage their output, ensuring robust and testable interactions with the external tool. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds the foundational components for an Anyscale CLI wrapper, including the main struct, installation check, and a command runner, along with comprehensive tests. The implementation is solid, but I have a few suggestions. In the main implementation, a debug print statement should be removed. In the tests, there's an opportunity to make path construction more robust. More importantly, I've identified and suggested a fix for a bug in the test logic that was causing an output check to be skipped in error scenarios.
| if tt.wantErr != nil { | ||
| if err == nil { | ||
| t.Fatal("expected error, got nil") | ||
| } | ||
| if errors.Is(tt.wantErr, errAnyscaleNotInstalled) { | ||
| if !errors.Is(err, errAnyscaleNotInstalled) { | ||
| t.Errorf("expected errAnyscaleNotInstalled, got: %v", err) | ||
| } | ||
| } else if !strings.Contains(err.Error(), tt.wantErr.Error()) { | ||
| t.Errorf("error %q should contain %q", err.Error(), tt.wantErr.Error()) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if tt.wantSubstr != "" && !strings.Contains(output, tt.wantSubstr) { | ||
| t.Errorf("output %q should contain %q", output, tt.wantSubstr) | ||
| } |
There was a problem hiding this comment.
The current test logic for checking errors has a bug. The return statement on line 103 prevents the wantSubstr check on line 109 from running for test cases that expect an error. This means the output check for the "command fails with stderr" case is skipped. The logic should be refactored to ensure wantSubstr is checked for all cases.
if tt.wantErr != nil {
if err == nil {
t.Fatal("expected error, got nil")
}
if errors.Is(tt.wantErr, errAnyscaleNotInstalled) {
if !errors.Is(err, errAnyscaleNotInstalled) {
t.Errorf("expected errAnyscaleNotInstalled, got: %v", err)
}
} else if !strings.Contains(err.Error(), tt.wantErr.Error()) {
t.Errorf("error %q should contain %q", err.Error(), tt.wantErr.Error())
}
} else if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if tt.wantSubstr != "" && !strings.Contains(output, tt.wantSubstr) {
t.Errorf("output %q should contain %q", output, tt.wantSubstr)
}| return "", errAnyscaleNotInstalled | ||
| } | ||
|
|
||
| fmt.Println("anyscale cli args: ", args) |
There was a problem hiding this comment.
This fmt.Println appears to be a debug statement. It should be removed or replaced with a proper logging mechanism to avoid polluting the standard output in production environments. Such output can be noisy and might expose sensitive information.
| tmp := t.TempDir() | ||
|
|
||
| if script != "" { | ||
| mockScript := tmp + "/anyscale" |
There was a problem hiding this comment.
For constructing file paths, it's more robust and platform-independent to use path/filepath.Join instead of string concatenation. This avoids issues with path separators on different operating systems. You'll need to add "path/filepath" to your imports.
| mockScript := tmp + "/anyscale" | |
| mockScript := filepath.Join(tmp, "anyscale") |
Code Review 👍 Approved with suggestions 0 resolved / 2 findingsSolid foundational CLI wrapper with good test coverage. Main concern is unconditional debug output in production code that could leak CLI arguments.
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
rayapp/anyscale_cli.go
Outdated
| // Capture output while also displaying to terminal with colors | ||
| var outputBuf bytes.Buffer | ||
| cmd.Stdout = io.MultiWriter(os.Stdout, &outputBuf) | ||
| cmd.Stderr = io.MultiWriter(os.Stderr, &outputBuf) |
There was a problem hiding this comment.
Details
The line fmt.Println("anyscale cli args: ", args) unconditionally prints CLI arguments to stdout. This has several concerns:
- Noisy output: Every CLI invocation will print debug info, which pollutes the output when used in automation/scripts
- Potential information leakage: CLI arguments could contain sensitive information (tokens, secrets) that shouldn't be logged by default
- Not controllable: There's no way to disable this output without modifying the code
Suggested fix: Either remove the debug statement, or make it conditional based on a debug/verbose flag or environment variable:
if os.Getenv("RAYAPP_DEBUG") != "" {
fmt.Println("anyscale cli args: ", args)
}Or use a logging package with configurable levels.
Was this helpful? React with 👍 / 👎
elliot-barn
changed the title
|
I've addressed the code review feedback: Changes made:
All tests pass successfully. |
rayapp/anyscale_cli.go
Outdated
| cmd := exec.Command("anyscale", args...) | ||
|
|
||
| // Capture output while also displaying to terminal with colors | ||
| var outputBuf bytes.Buffer |
There was a problem hiding this comment.
you might want to set a limit (e.g. first / last several k of bytes), otherwise, this can easily run into out of memory.
|
|
||
| output, err := cli.runAnyscaleCLI(tt.args) | ||
|
|
||
| if tt.wantErr != nil { |
There was a problem hiding this comment.
yeah, this error checking logic feels a bit unclear.. what is this trying to do?
why is errAnyscaleNotInstalled type error having special treatment?
rayapp/anyscale_cli.go
Outdated
| output := outputBuf.Bytes() | ||
| if len(output) > maxOutputBufferSize { |
There was a problem hiding this comment.
at this point it already runs out of memory..
There was a problem hiding this comment.
Using a ring buffer now
rayapp/anyscale_cli.go
Outdated
| return tw.String(), nil | ||
| } | ||
|
|
||
| // tailWriter is a circular buffer that keeps the most recent `limit` |
There was a problem hiding this comment.
this looks ok. could you:
- split this tailwriter into another file and in a leading PR?
- rather than allocating a
limitlength buffer, could you usebytes.Bufferso that the initial and idle memory footprint is near zero? you can achieve this with twolimit/2bytes.Buffer.
Extract tailWriter into its own file with a bytes.Buffer-based double-buffer design. Initial memory footprint is near zero (no upfront 1MB allocation); buffers grow lazily as data is written. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add foundational components for Anyscale CLI wrapper: - AnyscaleCLI struct and NewAnyscaleCLI() constructor - errAnyscaleNotInstalled error sentinel for installation checks - isAnyscaleInstalled() helper to verify CLI availability - runAnyscaleCLI() base method for executing anyscale commands Includes comprehensive test coverage with setupMockAnyscale test helper. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
Replace bytes.Buffer with a fixed-size circular buffer (tailWriter) so memory usage never exceeds maxOutputBufferSize during command execution. The previous approach truncated after the command finished, allowing unbounded growth during long-running commands. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove the inline ring-buffer tailWriter from anyscale_cli.go and its tests from anyscale_cli_test.go. The tailWriter now lives in tail_writer.go (from the tailwriter-extract branch) with a lazy double-buffer implementation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
elliot-barn
force-pushed
the
anyscale-cli-pr1-core-infrastructure
branch
from
d51c0db to
829ba7b
Compare
2 participants
Add foundational components for Anyscale CLI wrapper:
Includes comprehensive test coverage with setupMockAnyscale test helper.