config: update fields, defaults, and required checks to match README#190
Open
yushan8 wants to merge 5 commits into
Open
config: update fields, defaults, and required checks to match README#190yushan8 wants to merge 5 commits into
yushan8 wants to merge 5 commits into
Conversation
2 tasks
yushan8
force-pushed
the
yushan/config-readme-followup
branch
from
00fd1e1 to
4c78c65
Compare
2 tasks
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>
liujsj
reviewed
Jul 13, 2026
Comment on lines
+32
to
+34
| var ( | ||
| _bzlmodEnabledDefault = true | ||
| ) |
There was a problem hiding this comment.
Why is this not in the const block too?
Contributor
Author
There was a problem hiding this comment.
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.
liujsj
approved these changes
Jul 13, 2026
yushan8
force-pushed
the
yushan/add-config-doc
branch
from
1879205 to
a56db34
Compare
yushan8
force-pushed
the
yushan/config-readme-followup
branch
from
a2efb18 to
abe84e5
Compare
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
force-pushed
the
yushan/add-config-doc
branch
from
a5dbe2e to
0588d25
Compare
yushan8
force-pushed
the
yushan/config-readme-followup
branch
from
abe84e5 to
80ed523
Compare
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>
This was referenced Jul 14, 2026
yushan8
force-pushed
the
yushan/config-readme-followup
branch
from
a39ca66 to
b154f1a
Compare
Summary: Intent: - This branch builds on #180 (docs: add config/README.md), which documented the intended shape of the YAML config surface. Several documented field names, defaults, and required-ness didn't match the actual Go structs and Parse() behavior; this aligns the code with the docs. Changes: - Renamed config fields to match config/README.md: worker_pool_size -> max_worker_pool_size, repo_manager_clone_path -> workspaces_root, chunking -> streaming (and its target_chunk_size/changed_target_chunk_size/ metadata_map_chunk_size -> max_num_targets/max_num_changed_targets/ max_num_metadata_entries), query_timeout -> query_timeout_seconds. - config.Parse now enforces service.workspaces_root as required (previously silently defaulted to a temp dir) and applies the documented defaults: bzlmod_enabled defaults to true, query_timeout_seconds defaults to 600, and the streaming.max_num_* fields default to 250/125/50000. - Added Go doc comments for the config fields documented in the README. - Updated all call sites (controller, example/main.go, orchestrator/native_orchestrator.go) and YAML fixtures for the renames. - Added config/config_test.go covering the new validation and defaults. --- <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>
FullHashRepos, ExcludedFiles, ExcludeExternalTargets, StreamBazelLogs, and WorkerRootPath aren't part of the documented config surface added in config/README.md. Flag them for a follow-up decision: either delete if unneeded, or document them.
… into example server native_orchestrator.go still referenced the pre-rename Service.Chunking field after rebasing onto main (git's line-based merge didn't catch the semantic rename to Service.Streaming). Also wire ChunkConfig through in example/main.go's controller.NewController call, which was silently never passing configured chunk sizes to the controller.
…command) Matches the existing convention elsewhere in ServiceConfig (repo_manager_clone_path, worker_root_path) and config/README.md's current documented field names: workspaces_root -> workspaces_root_path, bazel_command -> bazel_command_path. Pure rename, no behavior change.
Removes _defaultMaxNumTargets/_defaultMaxNumChangedTargets/_defaultMaxNumMetadataEntries and their Parse defaulting blocks — they no longer match config/README.md (which documents a single max_message_bytes) and are about to be deleted outright by the follow-up PR that lands the consolidation, so there's no reason to introduce them here first. Also answers a review question on why _bzlmodEnabledDefault sits outside the const block: it has to be a var so its address can be taken.
yushan8
force-pushed
the
yushan/add-config-doc
branch
from
0588d25 to
01ae7e8
Compare
yushan8
force-pushed
the
yushan/config-readme-followup
branch
from
b154f1a to
6eaa518
Compare
3 tasks
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
2 participants
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
Aligns config field names, defaults, and required checks with config/README.md from #180.
worker_pool_size→max_worker_pool_size,repo_manager_clone_path→workspaces_root,chunking→streaming(with sub-fields renamed tomax_num_targets/max_num_changed_targets/max_num_metadata_entries),query_timeout→query_timeout_seconds.config.Parsenow enforcesservice.workspaces_rootas required and applies documented defaults:bzlmod_enableddefaults totrue,query_timeout_secondsdefaults to600, streaming fields default to250/125/50000.config/config_test.gocovering new validation and defaults.Test plan
make buildmake testStack