feat(settings): add AppSettings.app_dns_strategy to proto - #8018
Open
mhotan wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new field documentation uses “Unset” in a way that conflicts with the proto’s SettingState.SETTING_STATE_UNSET semantics and should be reworded to avoid misleading API consumers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends the Flyte settings proto (flyteidl2/settings/settings_definition.proto) by adding a new AppSettings.app_dns_strategy setting to control how public app URLs are composed (shared DNS vs per-dataplane DNS), and regenerates the language stubs accordingly.
Changes:
- Added
StringSetting app_dns_strategy = 2toAppSettingsin the settings proto. - Regenerated Go and Python protobuf outputs (including Go validate stubs).
- Regenerated TypeScript protobuf-es output to expose the new field.
File summaries
| File | Description |
|---|---|
| flyteidl2/settings/settings_definition.proto | Adds app_dns_strategy to AppSettings with documentation on token values and behavior. |
| gen/go/flyteidl2/settings/settings_definition.pb.go | Regenerated Go types/accessors for the new AppDnsStrategy field. |
| gen/go/flyteidl2/settings/settings_definition.pb.validate.go | Regenerated Go validation to include embedded validation for AppDnsStrategy. |
| gen/python/flyteidl2/settings/settings_definition_pb2.py | Regenerated Python runtime descriptor + message definitions for the new field. |
| gen/python/flyteidl2/settings/settings_definition_pb2.pyi | Regenerated Python typing stubs to include app_dns_strategy. |
| gen/ts/flyteidl2/settings/settings_definition_pb.ts | Regenerated TS types and docs to include appDnsStrategy. |
Review details
Files not reviewed (3)
- gen/go/flyteidl2/settings/settings_definition.pb.go: Generated file
- gen/go/flyteidl2/settings/settings_definition.pb.validate.go: Generated file
- gen/python/flyteidl2/settings/settings_definition_pb2.py: Generated file
- Files reviewed: 3/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+198
to
+199
| // "edge_proxy" is reserved for a future single-balanced-DNS strategy and is not | ||
| // yet a valid value. Unset delegates to the parent scope, then defaults to shared. |
Adds a StringSetting app_dns_strategy to AppSettings (the org-level Union Apps
policy message), selecting how an app's public URL is composed relative to the
data plane serving it:
- "shared" (default; also unset/unknown/empty): one tenant-wide DNS name for
the app regardless of which data plane serves it (today's behavior).
- "dataplane_specific": a per-data-plane DNS name, so the same app can run on
more than one data plane at distinct URLs.
"edge_proxy" is reserved for a future single-balanced-DNS strategy and is not
yet a valid value. Regenerated go/python/rust/ts stubs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Michael Hotan <mike@union.ai>
mhotan
force-pushed
the
mike/app-dns-strategy-setting
branch
from
September 10, 2026 19:42
2fdaaea to
8c193be
Compare
Contributor
Author
Engineering Review: Plan Alignment & CorrectnessReviewed against Multi-Dataplane App Serving Plan (Item 1):
LGTM and ready to merge. |
Adds `string cluster` (field 16) to app Spec: pins an app to a named cluster instead of load-balancing across a cluster_pool. Mutually exclusive with cluster_pool (the control plane rejects setting both); empty falls back to cluster_pool (empty pool = the default pool). Regenerated go/python/ts stubs. (Rust stubs deferred to the canonical whole-tree regen — a scoped regen drifts the pyo3 type attributes.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Michael Hotan <mike@union.ai>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Remove the unrelated schema change and resolve the settings inheritance and merge issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (6)
- gen/go/flyteidl2/app/app_definition.pb.go: Generated file
- gen/go/flyteidl2/app/app_definition.pb.validate.go: Generated file
- gen/go/flyteidl2/settings/settings_definition.pb.go: Generated file
- gen/go/flyteidl2/settings/settings_definition.pb.validate.go: Generated file
- gen/python/flyteidl2/app/app_definition_pb2.py: Generated file
- gen/python/flyteidl2/settings/settings_definition_pb2.py: Generated file
Suppressed comments (1)
flyteidl2/settings/settings_definition.proto:199
SETTING_STATE_UNSETis defined above as explicitly stopping inheritance, not delegating to the parent. Calling it "Unset delegates" makes an explicit child UNSET indistinguishable from INHERIT; please document omitted/INHERIT as delegation and define UNSET as blocking the parent before falling back toshared.
// yet a valid value. Unset delegates to the parent scope, then defaults to shared.
- Files reviewed: 6/13 changed files
- Comments generated: 2
- Review effort level: Lite
| // setting both is rejected. When set, the control plane leases the app only to the | ||
| // named cluster; when empty, placement falls back to cluster_pool (empty pool = | ||
| // the default pool). | ||
| string cluster = 16; |
| // more than one data plane at distinct URLs. | ||
| // "edge_proxy" is reserved for a future single-balanced-DNS strategy and is not | ||
| // yet a valid value. Unset delegates to the parent scope, then defaults to shared. | ||
| StringSetting app_dns_strategy = 2 [(flyteidl2.settings.desc) = "App public-URL DNS strategy: 'shared' (default) or 'dataplane_specific'"]; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a
StringSetting app_dns_strategy(field 2) to theAppSettingsmessage inflyteidl2/settings/settings_definition.proto, alongside the existing org-leveldisallow_anonymousUnion Apps policy.It selects how an app's public URL is composed relative to the data plane serving it:
shared(default; also unset / unknown / empty) — one tenant-wide DNS name for the app regardless of which data plane serves it (today's behavior).dataplane_specific— a per-data-plane DNS name, so the same app can run on more than one data plane at distinct URLs.edge_proxyis reserved for a future single-balanced-DNS strategy and is intentionally not yet a valid value (unknown values resolve toshared).A
StringSetting(not a bool) is used so the future strategy can slot in as a token with no schema change, mirroring how other settings evolve.Changes
app_dns_strategyfield onAppSettings.Consumer
The Union control plane reads this setting at app-lease time to compose per-cluster app URLs (multi-dataplane app serving). Backward compatible — additive field, default
sharedpreserves current behavior.🤖 Generated with Claude Code