Skip to content

[RAPTOR 18594] Schema change: preflight validation#679

Open
brunoromano-dr wants to merge 5 commits into
datarobot-oss:mainfrom
brunoromano-dr:RAPTOR-18594-schema-change-preflight-validation
Open

[RAPTOR 18594] Schema change: preflight validation#679
brunoromano-dr wants to merge 5 commits into
datarobot-oss:mainfrom
brunoromano-dr:RAPTOR-18594-schema-change-preflight-validation

Conversation

@brunoromano-dr

@brunoromano-dr brunoromano-dr commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

RATIONALE

Adds client-side preflight validation for dr workload create so users get a clear CLI error when a spec sets both replicaCount and autoscaling.enabled: true on the same container group — instead of waiting for a WAPI 422.

Follows workload-api#984 mutual-exclusivity rule. Builds on RAPTOR-18591 (docs/examples for the new autoscaling shape). Specs are still forwarded verbatim to WAPI after validation passes.

Ticket: RAPTOR-18594

CHANGES

File Change
internal/workload/workload.go validateRuntimeReplicaAutoscaling walks runtime.containerGroups[]; errors when replicaCount is set and autoscaling.enabled is true (default true when omitted)
internal/workload/workload_test.go Unit tests: fixed replica only, autoscaling only, replicaCount: null, autoscaling disabled + replicaCount, conflict cases with group index

Validation rule (per container group):

replicaCount autoscaling.enabled CLI result
set true (or omitted on autoscaling block) Error
set false OK
omitted / null true OK
set no autoscaling block OK

Explicitly deferred: bounds validation (maxReplicaCount >= 1), per-policy legacy field rejection — still handled by WAPI 422.

TESTING

GOPROXY=https://proxy.golang.org,direct go test ./internal/workload/ -run TestValidateWorkloadCreateRequest -v

Manual (should fail before API call):

export DATAROBOT_CLI_FEATURE_WORKLOAD=true
./dr workload create --spec-file /tmp/bad-spec.json
# spec with replicaCount + autoscaling.enabled: true → CLI error, no POST

RELATED

  • Depends on / pairs with: RAPTOR-18591 — autoscaling docs and docs/examples/workload-autoscaling.yaml
  • WAPI: workload-api#984_reconcile_replica_count in runtime.py

PR Automation

Comment-Commands: Trigger CI by commenting on the PR:

  • /trigger-smoke-test or /trigger-test-smoke - Run smoke tests
  • /trigger-install-test or /trigger-test-install - Run installation tests

Labels: Apply labels to trigger workflows:

  • run-smoke-tests or go - Run smoke tests on demand (only works for non-forked PRs)

Important

For Forked PRs: The run-smoke-tests label won't work. A required Smoke Tests check will block merge until a maintainer acts:

  • A maintainer uses /approve-smoke-tests to run smoke tests (results will set the check)
  • A maintainer uses /skip-smoke-tests to bypass the check without running tests

Please comment requesting a maintainer review if you need smoke tests to run.

Branch: RAPTOR-18594-schema-change-preflight-validationmain (non-forked; merge after RAPTOR-18591 or rebase onto it)

Diff size: 2 files (workload.go, workload_test.go)


Note

Low Risk
Local spec validation and documentation only; no change to successful create behavior or server contracts beyond catching invalid specs earlier.

Overview
Adds client-side preflight on dr workload create so specs that set both replicaCount and autoscaling.enabled: true on the same container group fail immediately with a clear path-indexed error, aligned with workload-api mutual-exclusivity instead of a late 422.

ValidateWorkloadCreateRequest now walks runtime.containerGroups[] and treats an autoscaling block with omitted enabled as enabled. Valid combinations (fixed replicas only, autoscaling only, replicaCount: null, or autoscaling.enabled: false with replicas) still pass; the payload is still sent verbatim when validation succeeds.

Docs and help are updated for a third create flow (autoscaling with bounds on autoscaling), a copy-paste docs/examples/workload-autoscaling.yaml, and expanded 422 examples. Unit tests cover the new rules and group index in errors.

Reviewed by Cursor Bugbot for commit 4217cac. Configure here.

@datarobot-pr-review-router

Copy link
Copy Markdown

👋 Thanks so much for contributing to the DataRobot community!

As a quick heads-up on how our team handles reviews: if you're still iterating on
this code or running tests, please feel free to convert this to a Draft PR.
We rely heavily on GitHub Drafts to give contributors a stress-free sandbox to experiment!

Once everything is finalized and you're ready for feedback, just click "Ready for review"
and the maintainers will be notified to jump in. (And if this PR is already 100% ready
to go, no action needed, we'll take a look soon!)

@datarobot-pr-review-router

Copy link
Copy Markdown

Code Ownership

Workload Cli

  • cmd/workload/create/cmd.go
  • internal/workload/workload.go
  • internal/workload/workload_test.go

Cli Maintainers

  • docs/commands/workload.md
  • docs/examples/workload-autoscaling.yaml

Review requested from the teams above. Labels will be removed automatically upon approval.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 4217cac. Configure here.

enabled, ok := autoscaling["enabled"]
if !ok {
return true
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

enabled: null bypasses autoscaling conflict detection

Low Severity

autoscalingEnabled treats "enabled": null differently from an absent "enabled" key. When the key is absent, it correctly defaults to true. When the key is null, the nil interface falls through to the bool type assertion and returns false instead, so the conflict with replicaCount goes undetected. The sibling function replicaCountSet explicitly handles null via v != nil, but that same pattern wasn't applied here. The server likely treats null and absent equivalently (both default to true), so the user gets a server 422 instead of the intended clear CLI error.

Fix in Cursor Fix in Web

Triggered by project rule: Bugbot Rules for DataRobot CLI

Reviewed by Cursor Bugbot for commit 4217cac. Configure here.

@chasdr chasdr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍

Comment thread docs/commands/workload.md
| `404` | The workload does not exist. |
| `409` | The workload must finish its current transition first (for example a `start` while it is still `stopping`). |
| `422` | The spec failed server validation; the response names the offending JSON path. |
| `422` | The spec failed server validation; the response names the offending JSON path (for example `replicaCount` set alongside `autoscaling.enabled: true`, or `maxReplicaCount` below 1). |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(not blocking) the 422 row still lists replicaCount + autoscaling.enabled: true as an example, but this PR now catches that combo client-side before the POST, so users hit the CLI preflight error, not a 422. drop that one and keep maxReplicaCount below 1 (still server-side)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants