Skip to content

fix: drop RunIdentifier.name pattern so ingest paths accept pre-existing run names - #7838

Merged
pvditt merged 2 commits into
mainfrom
pvditt/relax-run-name-ingest-validation
Aug 12, 2026
Merged

fix: drop RunIdentifier.name pattern so ingest paths accept pre-existing run names#7838
pvditt merged 2 commits into
mainfrom
pvditt/relax-run-name-ingest-validation

Conversation

@pvditt

@pvditt pvditt commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Tracking issue

Related to #7805 (ENG26-1081)

Why are the changes needed?

#7805 added run-name validation in two places: a validateRunName check at CreateRun, and a buf.validate pattern on RunIdentifier.name.

The proto pattern causes a problem: RunIdentifier is embedded in every message that references a run, including event reporting and status replication. In deployments that enforce protovalidate, runs created before #7805 rolled out fail that validation on every update — their events get rejected mid-run, so the runs look stuck while pods keep executing.

What changes were proposed in this pull request?

  • Remove the pattern from RunIdentifier.name (back to min/max length only) and regenerate.
  • Keep the CreateRun check — new invalid names are still rejected at creation, same as before.

How was this patch tested?

Check all the applicable boxes

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

🤖 Generated with Claude Code

https://claude.ai/code/session_01J9hFQMsdzZDKcgoJSVUnZr

…ing run names

PR #7805 added DNS-1035 run-name enforcement in two places: a validateRunName
check at CreateRun (the only external ingress for user-supplied names) and a
buf.validate pattern on RunIdentifier.name. The proto pattern was intended as
documentation, but in deployments where protovalidate is wired it is enforced
on every message embedding RunIdentifier — including event reporting and
record/status replication. Runs created before the rule existed became
un-ingestable mid-run: their updates were rejected with InvalidArgument and
retried or dropped, leaving the runs looking stuck while pods kept executing.

Remove the pattern (back to min_len/max_len) and keep validateRunName as the
single enforcement point. New invalid names are still rejected at CreateRun;
identifiers on read/report paths accept whatever was already created.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J9hFQMsdzZDKcgoJSVUnZr
Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
Copilot AI lite review requested due to automatic review settings August 12, 2026 06:32

Copilot AI 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.

Pull request overview

This PR removes the proto-level DNS-1035 regex constraint from RunIdentifier.name so that environments with protovalidate enabled don’t reject ingest/reporting/status updates for runs created before the stricter rule existed, while keeping creation-time enforcement in the RunService (validateRunName).

Changes:

  • Relax RunIdentifier.name validation in flyteidl2/common/identifier.proto to length-only (min 1, max 30).
  • Regenerate Go/Python/TypeScript outputs to reflect the removed pattern constraint.
  • Regenerate swagger output to reflect the updated identifier schema.

Reviewed changes

Copilot reviewed 3 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
flyteidl2/common/identifier.proto Drops the pattern validation on RunIdentifier.name, keeping only length constraints.
gen/go/flyteidl2/common/identifier.pb.go Regenerated Go protobuf output reflecting the updated validation metadata/docs.
gen/python/flyteidl2/common/identifier_pb2.py Regenerated Python protobuf output reflecting the updated validation metadata.
gen/ts/flyteidl2/common/identifier_pb.ts Regenerated TS protobuf output reflecting the updated validation metadata/docs.
gen/go/gateway/flyteidl2/connector/service.swagger.json Regenerated swagger output reflecting the updated identifier schema.
Files not reviewed (2)
  • gen/go/flyteidl2/common/identifier.pb.go: Generated file
  • gen/python/flyteidl2/common/identifier_pb2.py: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +97 to +100
string name = 4 [
(buf.validate.field).string.min_len = 1,
(buf.validate.field).string.max_len = 30
];
Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
Copilot AI review requested due to automatic review settings August 12, 2026 06:38

Copilot AI 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.

Pull request overview

Copilot reviewed 3 out of 6 changed files in this pull request and generated no new comments.

Files not reviewed (2)
  • gen/go/flyteidl2/common/identifier.pb.go: Generated file
  • gen/python/flyteidl2/common/identifier_pb2.py: Generated file
Suppressed comments (1)

flyteidl2/common/identifier.proto:101

  • After removing the DNS-1035 pattern and the explanatory comment here, the public proto/API docs no longer describe the run-name format that the server still enforces on RunService.CreateRun (runs/service/run_service.go calls validateRunName, which requires DNS-1035 and <=30 chars per runs/service/utils.go). This creates a doc/behavior mismatch for clients: CreateRun will still reject names like "my.run" even though RunIdentifier.name now only advertises min/max length.

Consider adding a short note clarifying that CreateRun enforces DNS-1035 for newly-created runs, while RunIdentifier may reference legacy runs that don't conform (hence no pattern on the identifier type).

  // Name of the run. Must be unique across all runs in this org, project, and domain pairing.
  string name = 4 [
    (buf.validate.field).string.min_len = 1,
    (buf.validate.field).string.max_len = 30
  ];

@pvditt
pvditt merged commit 1d94055 into main Aug 12, 2026
25 checks passed
@pvditt
pvditt deleted the pvditt/relax-run-name-ingest-validation branch August 12, 2026 06:51
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.

3 participants