Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build_rayapp.sh
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@ echo "--- Building rayapp"
mkdir -p ci/bin
go build -o ci/bin/rayapp ./rayapp/rayapp

echo "--- Building template releases"
echo "--- Running rayapp commands"
rm -rf _build
exec ci/bin/rayapp "$@"
35 changes: 35 additions & 0 deletions rayapp/anyscale_cli.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package rayapp

import (
"bytes"
"errors"
"fmt"
"os/exec"
)

var errAnyscaleNotInstalled = errors.New("anyscale is not installed")

func isAnyscaleInstalled() bool {
_, err := exec.LookPath("anyscale")
return err == nil
}

// RunAnyscaleCLI runs the anyscale CLI with the given arguments.
func RunAnyscaleCLI(args []string) (string, error) {
if !isAnyscaleInstalled() {
return "", errAnyscaleNotInstalled
}

cmd := exec.Command("anyscale", args...)

var stdout, stderr bytes.Buffer
cmd.Stdout = &stdout
cmd.Stderr = &stderr

err := cmd.Run()
if err != nil {
return "", fmt.Errorf("anyscale error: %v\nstderr: %s", err, stderr.String())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The error formatting could be improved for better diagnostics and to follow modern Go practices. Using %w to wrap the error allows for inspection of the error chain with errors.Is or errors.As. Also, including a newline \n in the error message can make logs harder to parse. It's generally better to have single-line error messages.

Suggested change
return "", fmt.Errorf("anyscale error: %v\nstderr: %s", err, stderr.String())
return "", fmt.Errorf("anyscale error: %w, stderr: %s", err, stderr.String())

}

return stdout.String(), nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The file should end with a newline character. This is a common convention and is enforced by many editors and tools to prevent issues with file concatenation and processing.

110 changes: 110 additions & 0 deletions rayapp/anyscale_cli_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
package rayapp

import (
"errors"
"os"
"strings"
"testing"
)

// setupMockAnyscale creates a mock anyscale script and returns a cleanup function.
func setupMockAnyscale(t *testing.T, script string) {
t.Helper()
tmp := t.TempDir()

if script != "" {
mockScript := tmp + "/anyscale"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For constructing file paths, it's better to use filepath.Join instead of manual string concatenation with /. This makes the code more portable and works correctly on different operating systems, like Windows. You'll need to add import "path/filepath" to the file.

Suggested change
mockScript := tmp + "/anyscale"
mockScript := filepath.Join(tmp, "anyscale")

if err := os.WriteFile(mockScript, []byte(script), 0755); err != nil {
t.Fatalf("failed to create mock script: %v", err)
}
}

origPath := os.Getenv("PATH")
t.Cleanup(func() { os.Setenv("PATH", origPath) })
os.Setenv("PATH", tmp)
}

func TestRunAnyscaleCLI(t *testing.T) {
tests := []struct {
name string
script string
args []string
wantErr error
wantSubstr string
}{
{
name: "anyscale not installed",
script: "", // empty PATH, no script
args: []string{"--version"},
wantErr: errAnyscaleNotInstalled,
},
{
name: "success",
script: "#!/bin/sh\necho \"output: $@\"",
args: []string{"service", "deploy"},
wantSubstr: "output: service deploy",
},
{
name: "empty args",
script: "#!/bin/sh\necho \"help\"",
args: []string{},
wantSubstr: "help",
},
{
name: "command fails with stderr",
script: "#!/bin/sh\necho \"error msg\" >&2; exit 1",
args: []string{"deploy"},
wantSubstr: "error msg",
wantErr: errors.New("anyscale error"),
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
setupMockAnyscale(t, tt.script)

output, err := RunAnyscaleCLI(tt.args)

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())
}
Comment on lines +188 to +194
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The error checking logic is a bit complex and can be simplified for better readability and maintainability. Specifically, errors.Is(tt.wantErr, errAnyscaleNotInstalled) is an unconventional way to check if tt.wantErr is of a certain type. A direct comparison tt.wantErr == errAnyscaleNotInstalled is more idiomatic here.

Suggested change
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())
}
if 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())
}

// For error cases, also check wantSubstr against error message
if tt.wantSubstr != "" && !strings.Contains(err.Error(), tt.wantSubstr) {
t.Errorf("error %q should contain %q", err.Error(), tt.wantSubstr)
}
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)
}
})
}
}

func TestIsAnyscaleInstalled(t *testing.T) {
t.Run("not installed", func(t *testing.T) {
setupMockAnyscale(t, "")
if isAnyscaleInstalled() {
t.Error("should return false when not in PATH")
}
})

t.Run("installed", func(t *testing.T) {
setupMockAnyscale(t, "#!/bin/sh\necho mock")
if !isAnyscaleInstalled() {
t.Error("should return true when in PATH")
}
})
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The file should end with a newline character. This is a common convention and is enforced by many editors and tools to prevent issues with file concatenation and processing.