-
Notifications
You must be signed in to change notification settings - Fork 127
Fix agent confusion about app commands (#30) #4045
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
Changes from all commits
afbee0c
f17143e
5ed5215
e5ed0b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,5 +15,6 @@ To get started, refer to the project README.md file and the documentation at htt | |
| >>> find my_pydabs -mindepth 1 ! -name pyproject.toml ! -regex .*/resources.* -delete | ||
|
|
||
| >>> ruff format --quiet --diff --check my_pydabs | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above, but you can just install |
||
| script: line 45: ruff: command not found | ||
|
|
||
| >>> yamlcheck.py | ||
| Exit code: 127 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,10 @@ package validation | |
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "time" | ||
|
|
||
| "github.com/databricks/cli/libs/log" | ||
|
|
@@ -11,6 +14,27 @@ import ( | |
| // ValidationNodeJs implements validation for Node.js-based projects using build, type check, and tests. | ||
| type ValidationNodeJs struct{} | ||
|
|
||
| // PackageJSON represents package.json structure | ||
| type PackageJSON struct { | ||
| Scripts map[string]string `json:"scripts"` | ||
| } | ||
|
|
||
| // readPackageJSON reads and parses package.json | ||
| func readPackageJSON(workDir string) (*PackageJSON, error) { | ||
| pkgPath := filepath.Join(workDir, "package.json") | ||
| data, err := os.ReadFile(pkgPath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read package.json: %w", err) | ||
| } | ||
|
|
||
| var pkg PackageJSON | ||
| if err := json.Unmarshal(data, &pkg); err != nil { | ||
| return nil, fmt.Errorf("failed to parse package.json: %w", err) | ||
| } | ||
|
|
||
| return &pkg, nil | ||
| } | ||
|
|
||
| type validationStep struct { | ||
| name string | ||
| command string | ||
|
|
@@ -19,39 +43,55 @@ type validationStep struct { | |
| } | ||
|
|
||
| func (v *ValidationNodeJs) Validate(ctx context.Context, workDir string) (*ValidateResult, error) { | ||
| log.Info(ctx, "Starting Node.js validation: build + typecheck + tests") | ||
| log.Info(ctx, "Starting Node.js validation") | ||
| startTime := time.Now() | ||
| var progressLog []string | ||
|
|
||
| progressLog = append(progressLog, "🔄 Starting Node.js validation: build + typecheck + tests") | ||
| progressLog = append(progressLog, "🔄 Starting Node.js validation") | ||
|
|
||
| // Read package.json | ||
| pkg, err := readPackageJSON(workDir) | ||
| if err != nil { | ||
| log.Warnf(ctx, "Could not read package.json: %v. Using defaults.", err) | ||
| progressLog = append(progressLog, "⚠️ Could not read package.json, using defaults") | ||
| pkg = &PackageJSON{Scripts: map[string]string{ | ||
| "build": "", "typecheck": "", "test": "", | ||
| }} | ||
| } | ||
|
|
||
| // Build steps based on available scripts | ||
| steps := []validationStep{ | ||
| { | ||
| name: "install", | ||
| command: "npm install", | ||
| errorPrefix: "Failed to install dependencies", | ||
| displayName: "Install", | ||
| }, | ||
| { | ||
| name: "build", | ||
| command: "npm run build --if-present", | ||
| errorPrefix: "Failed to run npm build", | ||
| displayName: "Build", | ||
| }, | ||
| { | ||
| name: "typecheck", | ||
| command: "npm run typecheck --if-present", | ||
| errorPrefix: "Failed to run client typecheck", | ||
| displayName: "Type check", | ||
| }, | ||
| { | ||
| name: "tests", | ||
| command: "npm run test --if-present", | ||
| errorPrefix: "Failed to run tests", | ||
| displayName: "Tests", | ||
| }, | ||
| {name: "install", command: "npm install", errorPrefix: "Failed to install dependencies", displayName: "Install"}, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to parse AFAIK, this is functionally equivalent to the old implementation. The old implementation will always work even if thescrips are missing since we are running with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wanted to have a single source of truth, but I agree the code looks less clean , so will roll this back until I have cleaner idea of implementing this |
||
| } | ||
|
|
||
| if _, ok := pkg.Scripts["build"]; ok { | ||
| steps = append(steps, validationStep{ | ||
| name: "build", command: "npm run build", errorPrefix: "Failed to run build", displayName: "Build", | ||
| }) | ||
| progressLog = append(progressLog, "✓ Found 'build' script") | ||
| } else { | ||
| progressLog = append(progressLog, "⚠️ No 'build' script, skipping") | ||
| } | ||
|
|
||
| if _, ok := pkg.Scripts["typecheck"]; ok { | ||
| steps = append(steps, validationStep{ | ||
| name: "typecheck", command: "npm run typecheck", errorPrefix: "Failed typecheck", displayName: "Type check", | ||
| }) | ||
| progressLog = append(progressLog, "✓ Found 'typecheck' script") | ||
| } else { | ||
| progressLog = append(progressLog, "⚠️ No 'typecheck' script, skipping") | ||
| } | ||
|
|
||
| if _, ok := pkg.Scripts["test"]; ok { | ||
| steps = append(steps, validationStep{ | ||
| name: "test", command: "npm run test", errorPrefix: "Failed tests", displayName: "Tests", | ||
| }) | ||
| progressLog = append(progressLog, "✓ Found 'test' script") | ||
| } else { | ||
| progressLog = append(progressLog, "⚠️ No 'test' script, skipping") | ||
| } | ||
|
|
||
| // Execute steps | ||
| for i, step := range steps { | ||
| stepNum := fmt.Sprintf("%d/%d", i+1, len(steps)) | ||
| log.Infof(ctx, "step %s: running %s...", stepNum, step.name) | ||
|
|
@@ -61,28 +101,20 @@ func (v *ValidationNodeJs) Validate(ctx context.Context, workDir string) (*Valid | |
| err := runCommand(ctx, workDir, step.command) | ||
| if err != nil { | ||
| stepDuration := time.Since(stepStart) | ||
| log.Errorf(ctx, "%s failed (duration: %.1fs)", step.name, stepDuration.Seconds()) | ||
| log.Errorf(ctx, "%s failed (%.1fs)", step.name, stepDuration.Seconds()) | ||
| progressLog = append(progressLog, fmt.Sprintf("❌ %s failed (%.1fs)", step.displayName, stepDuration.Seconds())) | ||
| return &ValidateResult{ | ||
| Success: false, | ||
| Message: step.errorPrefix, | ||
| Details: err, | ||
| ProgressLog: progressLog, | ||
| Success: false, Message: step.errorPrefix, Details: err, ProgressLog: progressLog, | ||
| }, nil | ||
| } | ||
| stepDuration := time.Since(stepStart) | ||
| log.Infof(ctx, "✓ %s passed: duration=%.1fs", step.name, stepDuration.Seconds()) | ||
| log.Infof(ctx, "✓ %s passed: %.1fs", step.name, stepDuration.Seconds()) | ||
| progressLog = append(progressLog, fmt.Sprintf("✅ %s passed (%.1fs)", step.displayName, stepDuration.Seconds())) | ||
| } | ||
|
|
||
| totalDuration := time.Since(startTime) | ||
| log.Infof(ctx, "✓ all validation checks passed: total_duration=%.1fs, steps=%s", | ||
| totalDuration.Seconds(), "build + type check + tests") | ||
| log.Infof(ctx, "✓ all validation passed: %.1fs", totalDuration.Seconds()) | ||
| progressLog = append(progressLog, fmt.Sprintf("✅ All checks passed! Total: %.1fs", totalDuration.Seconds())) | ||
|
|
||
| return &ValidateResult{ | ||
| Success: true, | ||
| Message: "All validation checks passed", | ||
| ProgressLog: progressLog, | ||
| }, nil | ||
| return &ValidateResult{Success: true, Message: "All validation checks passed", ProgressLog: progressLog}, nil | ||
| } | ||
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.
This seems to be specific output to your local run and will cause tests to fail on CI, could you revert?