config: add _path suffix to path-like fields#202
Closed
yushan8 wants to merge 5 commits into
Closed
Conversation
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>
The <workspaces_root>/.workers derivation moved from a config.Parse default into example/main.go when ServiceConfig.WorkerRootPath was removed, but it was untested there (package main). Move it into config as a small method with its own test, so the layout documented on WorkspacesRoot can't drift silently again.
ServiceConfig.WorkerRootPath() recomputed the join on every call and exposed a public method just to derive a path. Set it once as a plain field during Parse instead, so every config initialization gets a consistent, precomputed value without callers needing to call a method.
…oval
Removing RepositoryConfig.ExcludeExternalTargets left the Bazel query
always including //external:all-targets, which errors under Bzlmod
("no such package 'external'") since this repo's own config sets
bzlmod_enabled: true. Gate the //external:all-targets clause on
BzlmodEnabled instead, matching the behavior documented in
config/README.md.
Removing ExcludedFiles also dropped the "^@@?bazel_tools/" exclusion
the integration test relied on to skip hashing @bazel_tools' files,
some of which (e.g. tools/test/tw.exe, Windows-only) don't exist on
every OS. Restore the equivalent exclusion via the per-request
RequestOptions.ExtraExcludeFilesRegex, which still exists precisely
for this kind of ad hoc exclusion.
…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.
3 tasks
yushan8
force-pushed
the
yushan/remove-undocumented-config-fields
branch
from
27cc6f7 to
67934f9
Compare
Contributor
Author
|
Folded directly into #190 instead of keeping this as a separate stacked PR. Closing. |
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
1 participant
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
ServiceConfig.WorkspacesRoot(workspaces_root) →WorkspacesRootPath(workspaces_root_path) andRepositoryConfig.BazelCommand(bazel_command) →BazelCommandPath(bazel_command_path)._pathsuffix convention already used elsewhere inServiceConfig(repo_manager_clone_path,worker_root_path) and the currentconfig/README.md, per review feedback on [config] Tango configuration documentation #180.Stacked on #191.
Test plan
make buildmake testmake gazelle