feat: add argo-workflows v4.0.5 as OCM component#89
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces Argo Workflows v4.0.5 as a new OCM component, complete with component definition, Helm configurations for minimal and production deployments, docs, CI integration, and end-to-end tests for both deployment profiles. ChangesArgo Workflows OCM Component
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
argo-workflows/tests/test-production.sh (1)
135-135: ⚡ Quick winUse
NAMESPACEconsistently instead of hardcodedargo.Line 9 defines
NAMESPACE, but Lines 135/182/190-191/197/205 hardcodeargo. This creates config drift and makes future namespace changes brittle.Proposed refactor
kubectl apply -f - <<'EOF' apiVersion: argoproj.io/v1alpha1 kind: Workflow metadata: name: dag-parallel-test - namespace: argo spec: serviceAccountName: argo-workflow-PHASE=$(kubectl get workflow dag-parallel-test -n argo -o jsonpath='{.status.phase}' 2>/dev/null || echo "") +PHASE=$(kubectl get workflow dag-parallel-test -n "${NAMESPACE}" -o jsonpath='{.status.phase}' 2>/dev/null || echo "") ... - kubectl describe workflow dag-parallel-test -n argo - kubectl get pods -n argo -l workflows.argoproj.io/workflow=dag-parallel-test -o wide + kubectl describe workflow dag-parallel-test -n "${NAMESPACE}" + kubectl get pods -n "${NAMESPACE}" -l workflows.argoproj.io/workflow=dag-parallel-test -o wide ... - kubectl describe workflow dag-parallel-test -n argo + kubectl describe workflow dag-parallel-test -n "${NAMESPACE}" ... -FAILED_NODES=$(kubectl get workflow dag-parallel-test -n argo \ +FAILED_NODES=$(kubectl get workflow dag-parallel-test -n "${NAMESPACE}" \Also applies to: 182-182, 190-191, 197-197, 205-205
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@argo-workflows/tests/test-production.sh` at line 135, Replace all hardcoded occurrences of the literal namespace "argo" with the NAMESPACE variable so the script uses the configured namespace consistently; specifically, change YAML entries like `namespace: argo` and any CLI invocations or manifests that reference "argo" to use `namespace: $NAMESPACE` or `-n "$NAMESPACE"` (or `${NAMESPACE}`) as appropriate, ensuring proper quoting/expansion where used (e.g., kubectl/argo commands and manifest templates) and update the occurrences currently hardcoded to "argo" so they reference the NAMESPACE variable instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@argo-workflows/tests/test-production.sh`:
- Around line 115-120: The script currently only warns when no
PodDisruptionBudgets are found; change the behavior to fail fast by checking the
PDB_COUNT variable and, when PDB_COUNT is less than 1, call the error path
instead of log_warn and exit with a non-zero status; specifically replace the
block that uses PDB_COUNT and log_warn with logic that invokes log_error (or an
equivalent error logging helper) and exits (e.g., exit 1) so tests fail when no
PDBs are present.
- Around line 124-127: Add an assertion after computing CONTROLLER_NODES to
enforce HA: check if the CONTROLLER_NODES variable is less than 2 and, if so,
log an error and exit non‑zero; for example, use an if [ "$CONTROLLER_NODES" -lt
2 ] check, call log_error with a clear message about insufficient controller pod
spread, and exit 1; place this immediately after the existing CONTROLLER_NODES
assignment and the log_info call so the script fails when controller pods are
not spread across at least 2 nodes.
---
Nitpick comments:
In `@argo-workflows/tests/test-production.sh`:
- Line 135: Replace all hardcoded occurrences of the literal namespace "argo"
with the NAMESPACE variable so the script uses the configured namespace
consistently; specifically, change YAML entries like `namespace: argo` and any
CLI invocations or manifests that reference "argo" to use `namespace:
$NAMESPACE` or `-n "$NAMESPACE"` (or `${NAMESPACE}`) as appropriate, ensuring
proper quoting/expansion where used (e.g., kubectl/argo commands and manifest
templates) and update the occurrences currently hardcoded to "argo" so they
reference the NAMESPACE variable instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce890965-145d-4ead-8075-1ee7f3e1f846
📒 Files selected for processing (8)
.github/workflows/release-ocm-components.ymlREADME.mdargo-workflows/README.mdargo-workflows/component-constructor.yamlargo-workflows/minimal-values.yamlargo-workflows/production-values.yamlargo-workflows/tests/test-minimal.shargo-workflows/tests/test-production.sh
Thanks for pointing them out @coderabbitai
What
Add Argo Workflows v4.0.5 as an OCM component.
Why
Argo Workflows is a runtime dependency of artifact-conduit. Packaging it as an OCM component makes it distributable and deployable in air-gapped environments alongside the rest of the stack.
Testing
bash argo-workflows/tests/test-minimal.sh— kind cluster, single-replica install, hello-world workflow runs toSucceededbash argo-workflows/tests/test-production.sh— 4-node kind cluster, 2-replica HA install, PDBs verified, DAG parallel workflow runs toSucceededBoth tests pass 100%.
Notes for reviewers
workflow.serviceAccount.create: trueandworkflow.rbac.create: trueare required in both value profiles — without them, workflow executor pods cannot post results back (workflowtaskresultsRBAC) and every workflow fails withError.authModes: [client](Kubernetes RBAC) as the secure default; SSO/OIDC config is documented in comments inproduction-values.yamlandREADME.md.helm search repo argo/argo-workflows --versions).Checklist
Summary by CodeRabbit