Skip to content

feat(runs): validate settings on create and update - #7900

Merged
popojk merged 4 commits into
flyteorg:mainfrom
davidlin20dev:feat/settings-validators
Aug 27, 2026
Merged

popojk merged 4 commits into
flyteorg:mainfrom
davidlin20dev:feat/settings-validators

Conversation

@davidlin20dev

Copy link
Copy Markdown
Contributor

Tracking issue

Related to #7775. This is task 2.3 (validators).

Why are the changes needed?

The buf.validate annotations on the settings protos are not enforced by the generated Go code, so the service currently accepts any value. task_resource.max.memory can be stored as "apple". Nothing reads these values yet, but once the appliers land a bad value would fail deep inside a run instead of at the point someone saved it.

What changes were proposed in this pull request?

Validators wired into CreateSettings and UpdateSettings:

  • quantities must parse with resource.ParseQuantity, across all eight task_resource.{min,max}.{cpu,gpu,memory,storage} leaves; the error names the dot-path
  • max_action_concurrency must be 0 or between 2 and MaxUint32. The ceiling matches RunSpec.max_action_concurrency, which is a uint32
  • key-shape checks, previously duplicated in three handlers, extracted into validateSettingsKey

Leaves whose state is not VALUE are skipped, so sparse documents stay valid.

How was this patch tested?

Table tests per validator, plus rows added to the existing TestCreateSettings_Validation and TestUpdateSettings_Validation tables to cover the handler wiring.

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Follows #7841, #7859 and #7881.

Signed-off-by: davidlin20dev <davidlin20.dev@gmail.com>
Signed-off-by: davidlin20dev <davidlin20.dev@gmail.com>
…idators

Signed-off-by: davidlin20dev <davidlin20.dev@gmail.com>
Signed-off-by: davidlin20dev <davidlin20.dev@gmail.com>

@popojk popojk 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 thanks! cc @pingsutw for second review.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants