chore: implement argoapp agent#760
Conversation
📝 Walkthrough🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/workspace/jobagents/argo/argoapp.go`:
- Around line 100-113: getK8sCompatibleName currently trims edge characters
before truncation, misses lowercasing, and can return empty strings; fix it by:
in getK8sCompatibleName first lowercase the input, replace any character not in
[a-z0-9-] with '-' (use a regex or equivalent), then if length > 63 truncate to
63, then trim any leading/trailing '-' characters, and finally if the result is
empty produce a safe fallback (e.g., "a" or "default") and ensure the final
value is <=63 characters; update only getK8sCompatibleName to follow this
ordering and these rules.
In `@apps/workspace-engine/pkg/workspace/jobagents/types/types.go`:
- Around line 60-62: The Map() implementation for RenderContext is using the key
"workflowStep" while the RenderContext.WorkflowStep field has json:"step";
update Map() (the function that builds result map and calls structToMap on
WorkflowStep) to use the key "step" instead of "workflowStep" so the map keys
match the struct JSON tag (or alternatively change the struct tag to
"workflowStep" if you prefer that name); ensure the change touches the Map()
code path where it checks r.WorkflowStep != nil and assigns
result["workflowStep"] = structToMap(r.WorkflowStep).
🧹 Nitpick comments (2)
apps/workspace-engine/pkg/workspace/jobagents/types/types.go (1)
72-82: Consider logging or propagating marshaling errors.
structToMapsilently returnsnilon marshal/unmarshal failures. While these errors are unlikely for the oapi types, silentnilentries could cause confusing template rendering failures downstream.apps/workspace-engine/pkg/workspace/jobagents/argo/argoapp.go (1)
147-159: String-based error detection is fragile but pragmatic.This approach works but is sensitive to error message changes in ArgoCD. Consider checking gRPC status codes directly if issues arise.
| Inputs map[string]any `json:"inputs"` | ||
| } | ||
|
|
||
| func (r *RenderContext) Map() map[string]any { |
There was a problem hiding this comment.
if you convert it to json and read it map to struct map[string] it will auto do this
| jobAgentConfig := context.JobAgentConfig | ||
| serverAddr, apiKey, template, err := a.parseJobAgentConfig(jobAgentConfig) |
There was a problem hiding this comment.
your using the job agent config here from the context?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/workspace/jobagents/argo/argoapp.go`:
- Around line 170-182: The isRetryableError function currently only does literal
string matching; update it to detect gRPC status codes and network timeouts: use
status.FromError(err) to check for codes.Unavailable and codes.DeadlineExceeded
(and optionally codes.ResourceExhausted) and return true for those, assert if
err implements net.Error and err.Timeout() return true, and keep a fallback by
lowercasing err.Error() and doing case-insensitive contains checks for
"502","503","504","connection refused","connection reset","timeout","temporarily
unavailable"; reference the function isRetryableError and use status.FromError,
codes.Unavailable, codes.DeadlineExceeded, net.Error and strings.ToLower for the
changes.
| func isRetryableError(err error) bool { | ||
| if err == nil { | ||
| return false | ||
| } | ||
| errStr := err.Error() | ||
| // Check for HTTP status codes that indicate transient failures | ||
| return strings.Contains(errStr, "502") || | ||
| strings.Contains(errStr, "503") || | ||
| strings.Contains(errStr, "504") || | ||
| strings.Contains(errStr, "connection refused") || | ||
| strings.Contains(errStr, "connection reset") || | ||
| strings.Contains(errStr, "timeout") || | ||
| strings.Contains(errStr, "temporarily unavailable") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the argoapp.go file
fd -t f "argoapp.go" apps/workspace-engineRepository: ctrlplanedev/ctrlplane
Length of output: 127
🏁 Script executed:
# Check the file size and examine the imports and context around the function
wc -l apps/workspace-engine/pkg/workspace/jobagents/argo/argoapp.goRepository: ctrlplanedev/ctrlplane
Length of output: 131
🏁 Script executed:
# Look at the imports and the isRetryableError function in context
head -50 apps/workspace-engine/pkg/workspace/jobagents/argo/argoapp.goRepository: ctrlplanedev/ctrlplane
Length of output: 1311
🏁 Script executed:
# Check the specific function and surrounding context
sed -n '160,200p' apps/workspace-engine/pkg/workspace/jobagents/argo/argoapp.goRepository: ctrlplanedev/ctrlplane
Length of output: 720
🏁 Script executed:
# Search for where isRetryableError is being used
rg "isRetryableError" apps/workspace-engineRepository: ctrlplanedev/ctrlplane
Length of output: 701
🏁 Script executed:
# Check for ApplicationServiceClient usage and how it's called
rg "ApplicationServiceClient\|argocdapplication" apps/workspace-engine/pkg/workspace/jobagents/argo/ -A 3Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Look at the other isRetryableError implementation
sed -n '1,50p' apps/workspace-engine/pkg/workspace/jobdispatch/argocd.go | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 507
🏁 Script executed:
# Search for the pattern where ApplicationServiceClient is used
rg "CreateApplication\|UpdateApplication" apps/workspace-engine/pkg/workspace/jobagents/argo/argoapp.go -B 2 -A 5Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Look for where the client is created and used
rg "NewApplicationServiceClient\|applicationClient\|clientConn" apps/workspace-engine/pkg/workspace/jobagents/argo/argoapp.go -B 2 -A 3Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Check the full argoapp.go file to understand the error flow
sed -n '90,170p' apps/workspace-engine/pkg/workspace/jobagents/argo/argoapp.goRepository: ctrlplanedev/ctrlplane
Length of output: 2304
🏁 Script executed:
# Search web for Argo CD ApplicationServiceClient gRPC error handling patterns
web_search "Argo CD ApplicationServiceClient gRPC error codes Unavailable"Repository: ctrlplanedev/ctrlplane
Length of output: 116
🌐 Web query:
Argo CD v2 ApplicationServiceClient gRPC error handling
💡 Result:
For Argo CD v2 ApplicationServiceClient (Go gRPC client), handle errors the same way as any grpc-go client: unwrap the returned error into a gRPC Status and branch on the status code.
1) Unary RPCs (most ApplicationServiceClient calls)
resp, err := appClient.Get(ctx, req)
if err != nil {
st, ok := status.FromError(err) // or status.Convert(err)
if !ok { /* non-gRPC error */ }
switch st.Code() {
case codes.Unauthenticated, codes.PermissionDenied:
// auth/RBAC problem
case codes.NotFound:
// app doesn’t exist
case codes.DeadlineExceeded, codes.Unavailable:
// retry/backoff candidate
default:
// log st.Message()
}
}status.FromError / status.Code are the canonical helpers in grpc-go for extracting the code/message (and optional details). (pkg.go.dev)
2) Client-streaming RPCs (notably GetManifestsWithFiles)
Argo CD’s ApplicationService includes a client-streaming method GetManifestsWithFiles (Send(...) + CloseAndRecv()). (pkg.go.dev)
Pattern:
stream, err := appClient.GetManifestsWithFiles(ctx)
if err != nil { /* unwrap with status.FromError */ }
if err := stream.Send(chunk1); err != nil { /* unwrap */ }
if err := stream.Send(chunk2); err != nil { /* unwrap */ }
resp, err := stream.CloseAndRecv()
if err != nil { /* unwrap */ }Important: if you’re using gRPC-Web, it does not support client-side streaming, so this RPC (and CLI features that depend on it) will fail; use native gRPC (HTTP/2) instead. (github.com)
3) If you’re proxying via grpc-gateway
If you pass a custom ApplicationServiceClient into RegisterApplicationServiceHandlerClient, you are responsible for making sure interceptors/normal gRPC flow still happen (otherwise behavior can differ). (pkg.go.dev)
Citations:
- 1: https://pkg.go.dev/google.golang.org/grpc/status?utm_source=openai
- 2: https://pkg.go.dev/github.com/argoproj/argo-cd/v2/pkg/apiclient/application?utm_source=openai
- 3: Clarify --grpc-web limitations and propose unary alternative to client streaming argoproj/argo-cd#22773
- 4: https://pkg.go.dev/github.com/argoproj/argo-cd/v2/pkg/apiclient/application?utm_source=openai
🏁 Script executed:
# Search for ApplicationServiceClient Create method documentation
rg -t go "type ApplicationServiceClient" -A 20Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Check if there are any tests for isRetryableError that might show what errors it expects
cat apps/workspace-engine/pkg/workspace/jobdispatch/argocd_test.goRepository: ctrlplanedev/ctrlplane
Length of output: 35743
🏁 Script executed:
# Search for gRPC or status handling elsewhere in the codebase
rg "google.golang.org/grpc/status\|google.golang.org/grpc/codes" apps/workspace-engineRepository: ctrlplanedev/ctrlplane
Length of output: 48
Handle gRPC status codes from Argo CD ApplicationServiceClient.
Argo CD v2's ApplicationServiceClient is gRPC-based and returns errors as gRPC status codes (e.g., Unavailable, DeadlineExceeded). The current string-matching approach won't catch these codes, causing transient errors to fail without retrying. Check for gRPC status codes using status.FromError(), handle net.Error timeouts, and use case-insensitive string matching as a fallback.
🛠️ Suggested fix
import (
"bytes"
"context"
+ "errors"
"fmt"
"io"
+ "net"
"regexp"
"strings"
"time"
"workspace-engine/pkg/oapi"
"workspace-engine/pkg/templatefuncs"
"workspace-engine/pkg/workspace/jobagents/types"
"workspace-engine/pkg/workspace/store"
argocdclient "github.com/argoproj/argo-cd/v2/pkg/apiclient"
argocdapplication "github.com/argoproj/argo-cd/v2/pkg/apiclient/application"
"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
"github.com/avast/retry-go"
"github.com/charmbracelet/log"
"github.com/goccy/go-yaml"
+ "google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
)
func isRetryableError(err error) bool {
if err == nil {
return false
}
+ if st, ok := status.FromError(err); ok {
+ switch st.Code() {
+ case codes.Unavailable, codes.DeadlineExceeded:
+ return true
+ }
+ }
+
+ var netErr net.Error
+ if errors.As(err, &netErr) && netErr.Timeout() {
+ return true
+ }
+
- errStr := err.Error()
+ errStr := strings.ToLower(err.Error())
// Check for HTTP status codes and common network failures
return strings.Contains(errStr, "502") ||
strings.Contains(errStr, "503") ||
strings.Contains(errStr, "504") ||
strings.Contains(errStr, "connection refused") ||
strings.Contains(errStr, "connection reset") ||
strings.Contains(errStr, "timeout") ||
strings.Contains(errStr, "temporarily unavailable")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func isRetryableError(err error) bool { | |
| if err == nil { | |
| return false | |
| } | |
| errStr := err.Error() | |
| // Check for HTTP status codes that indicate transient failures | |
| return strings.Contains(errStr, "502") || | |
| strings.Contains(errStr, "503") || | |
| strings.Contains(errStr, "504") || | |
| strings.Contains(errStr, "connection refused") || | |
| strings.Contains(errStr, "connection reset") || | |
| strings.Contains(errStr, "timeout") || | |
| strings.Contains(errStr, "temporarily unavailable") | |
| func isRetryableError(err error) bool { | |
| if err == nil { | |
| return false | |
| } | |
| if st, ok := status.FromError(err); ok { | |
| switch st.Code() { | |
| case codes.Unavailable, codes.DeadlineExceeded: | |
| return true | |
| } | |
| } | |
| var netErr net.Error | |
| if errors.As(err, &netErr) && netErr.Timeout() { | |
| return true | |
| } | |
| errStr := strings.ToLower(err.Error()) | |
| // Check for HTTP status codes and common network failures | |
| return strings.Contains(errStr, "502") || | |
| strings.Contains(errStr, "503") || | |
| strings.Contains(errStr, "504") || | |
| strings.Contains(errStr, "connection refused") || | |
| strings.Contains(errStr, "connection reset") || | |
| strings.Contains(errStr, "timeout") || | |
| strings.Contains(errStr, "temporarily unavailable") | |
| } |
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/jobagents/argo/argoapp.go` around lines
170 - 182, The isRetryableError function currently only does literal string
matching; update it to detect gRPC status codes and network timeouts: use
status.FromError(err) to check for codes.Unavailable and codes.DeadlineExceeded
(and optionally codes.ResourceExhausted) and return true for those, assert if
err implements net.Error and err.Timeout() return true, and keep a fallback by
lowercasing err.Error() and doing case-insensitive contains checks for
"502","503","504","connection refused","connection reset","timeout","temporarily
unavailable"; reference the function isRetryableError and use status.FromError,
codes.Unavailable, codes.DeadlineExceeded, net.Error and strings.ToLower for the
changes.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.