Skip to content

feat(runs): add settings merge engine - #7925

Merged
popojk merged 7 commits into
flyteorg:mainfrom
davidlin20dev:feat/settings-merge
Aug 28, 2026
Merged

popojk merged 7 commits into
flyteorg:mainfrom
davidlin20dev:feat/settings-merge

Conversation

@davidlin20dev

Copy link
Copy Markdown
Contributor

Tracking issue

Related to #7775. This is task 2.2 (merge engine).

Why are the changes needed?

Settings are stored one row per scope, so answering "what applies to this project?" means resolving up to three rows into one document. Nothing does that yet, which is why GetSettings still returns Unimplemented and why no Phase 3 applier can start.

What changes were proposed in this pull request?

Pure resolution functions in runs/service/settings_merge.go, following the rules in the RFC:

  • scalars: walk org, domain, project; the last level whose state is not INHERIT wins, so UNSET at a child level blocks a value set above it
  • string maps: entries accumulate parent first, child entries overwrite on key conflict, and a level in state UNSET clears everything accumulated above it
  • task resources: the scalar rule applied per dimension across the min and max bounds
  • every resolved leaf is annotated with the scope_level it came from

mergeSettings is the only entry point. The rest are either leaf mergers, which hold the rules, or group mergers, which only regroup fields by level and delegate. Groups that no level configured resolve to nil, so the merged document carries no empty objects.

Nothing calls this yet. The GetSettings handler is the other half of task 2.5 and comes in a follow-up.

One point the RFC leaves open: it says to annotate the winner with its scope level, which is unambiguous for a scalar but not for a map assembled from several levels. Here a merged map records the most specific level that contributed. Per-entry origins remain available from GetSettingsForEdit.

How was this patch tested?

Table tests with no database, since the functions are pure. Every SettingState appears at every position in the chain, and the map table covers accumulation, child override, UNSET clearing, and a level below UNSET refilling the map. There is also a test that fails if a field is added to the Settings proto without being handled in mergeSettings.

Labels

  • added

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. Stacked on #7900.

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

Signed-off-by: davidlin20dev <davidlin20.dev@gmail.com>
Signed-off-by: davidlin20dev <davidlin20.dev@gmail.com>
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.

mergeString/Int64/Bool/Quantity looks similiar, can we do it in an abstract method? Something like:

type scalarSetting interface {
      proto.Message
      GetState() settings.SettingState
}

func mergeScalar[T scalarSetting](levels []T, setLevel func(T, settings.ScopeLevel)) T {
      var out T
      won := -1
      for i, s := range levels {
              if s.GetState() != settings.SettingState_SETTING_STATE_INHERIT {
                      won = i
              }
      }
      if won < 0 {
              return out // typed nil
      }
      out = proto.Clone(levels[won]).(T)
      setLevel(out, scopeLevelAt(won))
      return out
}

Comment thread runs/service/settings_merge.go Outdated
)

// scopeLevelAt maps a position in the level chain to the scope it represents.
// Callers build the chain broadest first: instance, domain, project.

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.

nit: should we say org i/o instance?

Suggested change
// Callers build the chain broadest first: instance, domain, project.
// Callers build the chain broadest first: org, domain, project.

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

Copy link
Copy Markdown
Contributor Author

@popojk Thanks for the review. Good call! I just updated it, ready for another look.

@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. Nice work!

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