Skip to content

config: update fields, defaults, and required checks to match README - #190

Merged
yushan8 merged 1 commit into
mainfrom
yushan/config-readme-followup
Aug 6, 2026
Merged

config: update fields, defaults, and required checks to match README#190
yushan8 merged 1 commit into
mainfrom
yushan/config-readme-followup

Conversation

@yushan8

@yushan8 yushan8 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Summary

Aligns config field names, defaults, and required checks with config/README.md from #180.

  • Renamed fields: worker_pool_sizemax_worker_pool_size, repo_manager_clone_pathworkspaces_root, chunkingstreaming (with sub-fields renamed to max_num_targets/max_num_changed_targets/max_num_metadata_entries), query_timeoutquery_timeout_seconds.
  • config.Parse now enforces service.workspaces_root as required and applies documented defaults: bzlmod_enabled defaults to true, query_timeout_seconds defaults to 600, streaming fields default to 250/125/50000.
  • Added Go doc comments for config fields documented in the README.
  • Updated all callsites and YAML fixtures for the renames.
  • Added config/config_test.go covering new validation and defaults.

Test plan

  • make build
  • make test

Stack

  1. [config] Tango configuration documentation #180
  2. @ config: update fields, defaults, and required checks to match README #190
  3. config: remove fields not documented in config/README.md #191

@yushan8
yushan8 force-pushed the yushan/config-readme-followup branch from 00fd1e1 to 4c78c65 Compare July 12, 2026 23:56
@yushan8
yushan8 marked this pull request as ready for review July 13, 2026 01:36
@yushan8
yushan8 requested review from a team as code owners July 13, 2026 01:36
yushan8 added a commit that referenced this pull request Jul 13, 2026
Summary:
Intent:
- This branch builds on #190 (config: update fields, defaults, and required
  checks to match README), which aligned names/defaults/required-ness with
  config/README.md but left several fields that the README doesn't document
  at all. This removes them so the Go structs match the documented surface
  exactly.

Changes:
- Removed RepositoryConfig.FullHashRepos, ExcludedFiles, ExcludeExternalTargets,
  and StreamBazelLogs, and ServiceConfig.WorkerRootPath.
- These fields drove real behavior, so this is a narrow behavior change, not
  just a doc cleanup: graphrunner/native.go now always includes external
  targets in the Bazel query and no longer applies a repo-level full-hash or
  excluded-file list (per-request excludes via RequestOptions still work),
  orchestrator/native_orchestrator.go no longer streams Bazel logs, and
  example/main.go derives the worker checkout path as
  workspaces_root/.workers instead of reading a separate config field.
- Updated config/config_test.go and the YAML fixtures
  (example/tango-config.yaml, orchestrator/testdata/config.yaml,
  integration/testdata/tango-config.yaml.tmpl) to drop the removed keys.

---

<sub>Generated by the 🪄 [pr-create](https://sg.uberinternal.com/code.uber.internal/uber-code/devexp-agent-marketplace/-/blob/claude-code/plugins/dev/uber-dev/skills/pr-create/SKILL.md) skill in devexp-agent-marketplace</sub>
Comment thread config/config.go Outdated
Comment on lines +32 to +34
var (
_bzlmodEnabledDefault = 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.

Why is this not in the const block too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's because the value is *bool not bool. Since the default is now true we need to make sure the value was unset. If we made it a bool, the default is false. There is no way to tell if it's false because it's unset or false because the user set it. So to check if bzlmod was disabled, we would check BzlmodEnabled != nil && *BzlmodEnabled == false.

@yushan8
yushan8 force-pushed the yushan/add-config-doc branch from 1879205 to a56db34 Compare July 13, 2026 23:38
@yushan8
yushan8 force-pushed the yushan/config-readme-followup branch from a2efb18 to abe84e5 Compare July 14, 2026 20:28
yushan8 added a commit that referenced this pull request Jul 14, 2026
Summary:
Intent:
- This branch builds on #190 (config: update fields, defaults, and required
  checks to match README), which aligned names/defaults/required-ness with
  config/README.md but left several fields that the README doesn't document
  at all. This removes them so the Go structs match the documented surface
  exactly.

Changes:
- Removed RepositoryConfig.FullHashRepos, ExcludedFiles, ExcludeExternalTargets,
  and StreamBazelLogs, and ServiceConfig.WorkerRootPath.
- These fields drove real behavior, so this is a narrow behavior change, not
  just a doc cleanup: graphrunner/native.go now always includes external
  targets in the Bazel query and no longer applies a repo-level full-hash or
  excluded-file list (per-request excludes via RequestOptions still work),
  orchestrator/native_orchestrator.go no longer streams Bazel logs, and
  example/main.go derives the worker checkout path as
  workspaces_root/.workers instead of reading a separate config field.
- Updated config/config_test.go and the YAML fixtures
  (example/tango-config.yaml, orchestrator/testdata/config.yaml,
  integration/testdata/tango-config.yaml.tmpl) to drop the removed keys.

---

<sub>Generated by the 🪄 [pr-create](https://sg.uberinternal.com/code.uber.internal/uber-code/devexp-agent-marketplace/-/blob/claude-code/plugins/dev/uber-dev/skills/pr-create/SKILL.md) skill in devexp-agent-marketplace</sub>
@yushan8
yushan8 force-pushed the yushan/add-config-doc branch from a5dbe2e to 0588d25 Compare July 14, 2026 20:31
@yushan8
yushan8 force-pushed the yushan/config-readme-followup branch from abe84e5 to 80ed523 Compare July 14, 2026 20:33
yushan8 added a commit that referenced this pull request Jul 14, 2026
Summary:
Intent:
- This branch builds on #190 (config: update fields, defaults, and required
  checks to match README), which aligned names/defaults/required-ness with
  config/README.md but left several fields that the README doesn't document
  at all. This removes them so the Go structs match the documented surface
  exactly.

Changes:
- Removed RepositoryConfig.FullHashRepos, ExcludedFiles, ExcludeExternalTargets,
  and StreamBazelLogs, and ServiceConfig.WorkerRootPath.
- These fields drove real behavior, so this is a narrow behavior change, not
  just a doc cleanup: graphrunner/native.go now always includes external
  targets in the Bazel query and no longer applies a repo-level full-hash or
  excluded-file list (per-request excludes via RequestOptions still work),
  orchestrator/native_orchestrator.go no longer streams Bazel logs, and
  example/main.go derives the worker checkout path as
  workspaces_root/.workers instead of reading a separate config field.
- Updated config/config_test.go and the YAML fixtures
  (example/tango-config.yaml, orchestrator/testdata/config.yaml,
  integration/testdata/tango-config.yaml.tmpl) to drop the removed keys.

---

<sub>Generated by the 🪄 [pr-create](https://sg.uberinternal.com/code.uber.internal/uber-code/devexp-agent-marketplace/-/blob/claude-code/plugins/dev/uber-dev/skills/pr-create/SKILL.md) skill in devexp-agent-marketplace</sub>
@yushan8
yushan8 force-pushed the yushan/config-readme-followup branch from a39ca66 to b154f1a Compare July 14, 2026 23:19
@yushan8
yushan8 force-pushed the yushan/add-config-doc branch from 0588d25 to 01ae7e8 Compare July 15, 2026 17:43
@yushan8
yushan8 force-pushed the yushan/config-readme-followup branch from b154f1a to 6eaa518 Compare July 15, 2026 17:44
Rebases PR #190's config-schema changes onto main, whose independent
config audit (2e0733f) already consolidated streaming defaults into
max_message_bytes, so that part is kept as-is. Only the naming/behavior
that main hadn't already converged on is applied:

- Service.WorkerPoolSize -> MaxWorkerPoolSize (max_worker_pool_size)
- Service.RepoManagerClonePath -> WorkspacesRootPath (workspaces_root_path),
  now required instead of defaulting to a temp dir
- RepositoryConfig.QueryTimeout -> QueryTimeoutSeconds (query_timeout_seconds),
  default lowered to 600s
- RepositoryConfig.BazelCommand -> BazelCommandPath (bazel_command_path)
- RepositoryConfig.BzlmodEnabled bool -> *bool, defaulting to true when unset
- TODO comments flagging FullHashRepos, ExcludedFiles, ExcludeExternalTargets,
  StreamBazelLogs, and WorkerRootPath as undocumented

Co-Authored-By: Claude Sonnet 5 <[email protected]>
@yushan8
yushan8 force-pushed the yushan/config-readme-followup branch from 6eaa518 to c7db8ee Compare August 6, 2026 21:42
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
4 out of 5 committers have signed the CLA.

✅ justinwon777
✅ xytan0056
✅ yushan8
✅ sywhang
❌ sbalabanov-zz
You have signed the CLA already but the status is still pending? Let us recheck it.

@yushan8
yushan8 changed the base branch from yushan/add-config-doc to main August 6, 2026 21:42
@yushan8
yushan8 merged commit 369f943 into main Aug 6, 2026
2 of 3 checks passed
@yushan8
yushan8 deleted the yushan/config-readme-followup branch August 6, 2026 21:51
yushan8 added a commit that referenced this pull request Aug 6, 2026
Rebases PR #191 onto the #190 config rework (currently on
yushan/config-readme-followup, itself not yet merged to main), dropping
ExcludeExternalTargets and the settable ServiceConfig.WorkerRootPath —
both undocumented and now redundant:

- ExcludeExternalTargets: graphrunner/native.go now derives the
  external-target query purely from BzlmodEnabled (bzlmod repos always
  query //external, so the separate toggle was redundant with it).
- WorkerRootPath: RepoManager already owns RepoManagerClonePath, so it
  derives its own <repo_manager_clone_path>/.workers/<repo>/worker-{1..N}/
  instead of taking a second, independently configurable path for the
  same tree.

FullHashRepos, ExcludedFiles, and StreamBazelLogs are kept for now (still
needed internally), with a TODO noting they're undocumented in
config/README.md.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
yushan8 added a commit that referenced this pull request Aug 6, 2026
Rebases PR #191 onto the #190 config rework (currently on
yushan/config-readme-followup, itself not yet merged to main), dropping
ExcludeExternalTargets and the settable ServiceConfig.WorkerRootPath —
both undocumented and now redundant:

- ExcludeExternalTargets: graphrunner/native.go now derives the
  external-target query purely from BzlmodEnabled (bzlmod repos always
  query //external, so the separate toggle was redundant with it).
- WorkerRootPath: RepoManager already owns RepoManagerClonePath, so it
  derives its own <repo_manager_clone_path>/.workers/<repo>/worker-{1..N}/
  instead of taking a second, independently configurable path for the
  same tree.

FullHashRepos, ExcludedFiles, and StreamBazelLogs are kept for now (still
needed internally), with a TODO noting they're undocumented in
config/README.md.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
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.

4 participants