Replace Makefile with Taskfile.yml and move all generation-related logic there#5050
Conversation
| generates: | ||
| - test-output.json | ||
| cmds: | ||
| - cat test-output-unit.json test-output-acc.json > test-output.json |
There was a problem hiding this comment.
cating two JSON doesn't create a JSON. Better to use jq here (ditto for others)
There was a problem hiding this comment.
these are json-lines files
There was a problem hiding this comment.
should we use .jsonl then?
There was a problem hiding this comment.
we could rename, but out of scope there (and might need updating other places like deco repo)
| sed -i 's|tagging.py|internal/genkit/tagging.py|g' .github/workflows/tagging.yml | ||
| fi | ||
| - "{{.GO_TOOL}} yamlfmt .github/workflows/tagging.yml" | ||
| - ./tools/validate_whitespace.py --fix |
There was a problem hiding this comment.
can we reference ws-fix task?
| generates: | ||
| - test-output.json | ||
| cmds: | ||
| - cat test-output-unit.json test-output-acc.json > test-output.json |
There was a problem hiding this comment.
should we use .jsonl then?
simonfaltum
left a comment
There was a problem hiding this comment.
Deep review (Isaac + Cursor, 2-round swarm)
Verdict: Not ready yet
Summary: 2 Critical | 9 Major | 2 Gap (Major) | 3 Nit | 4 Suggestion
Two independent reviewers (Isaac and Cursor) produced findings in parallel, then cross-reviewed each other's work, verifying claims against the code before agreeing / disagreeing. Inline comments follow below on specific lines. A few points that don't map cleanly to a single line:
Themes
- Makefile wrapper is lossy. Plain
makenow errors (.DEFAULTis a catch-all, not a default goal), andmake wsfixroutes to a non-existent task (renamed tows-fix). Either commit to a proper compat shim or drop the wrapper. - Windows CI path is broken.
./taskis a POSIX shell script; the Windows matrix legs inpush.ymldon't setshell: bashand default to pwsh. On main today these callmake(an.exe). - CI trigger graph is self-blind.
tools/testmaskonly treatsgo.mod/go.sum/setup-build-environment/as "common trigger" patterns. Changes toTaskfile.yml, thetasklauncher, ortools/testmask/**itself fall through totestonly, skipping specialized jobs (test-exp-aitools,test-exp-ssh,test-pipelines) precisely when their trigger or definition changed. Ironically this very PR would skip its own specialized tests if landed as-is. - Similarly for python CI:
python_push.ymlstill filters only onpaths: python/**but thepydabs-*task bodies now live in rootTaskfile.yml. generate-genkitshell rewrite subtly changes semantics. The old MakefileA && B || C && Dalways ranD(thegit checkout). The new groupedA && B || (C && D)skipsDwhen the SHA is already local — so genkit can build from whatever revision the user has checked out. Minimal shell repro confirmed.default/fulldev loops mutate goldens. They invoketest-update, nottest. Bare./taskcan silently bless acceptance-output regressions.- Cache source-list patterns are often wrong:
ROOT_LINT_SOURCESomits rootgo.mod/go.sum;pydabs-codegenhas overlappingsources:andgenerates:;generate-genkitsources missgo.mod/go.sumthat its embedded consistency assertion reads.
Findings dropped on cross-review (for transparency)
./task lintdropping--timeout=15m— verified:golangci-lint v2.5.0's--timeoutis now "Disabled by default," so no regression.generate-refschemalosing the old-ignore-errors prefix — verified:libs/testdiff's-updatemode overwrites goldens on mismatch rather than failing, so the-was masking real errors, not providing needed resilience../task --force generate-schemain CI — correct pattern forgenerator && git diff --exit-codeverification jobs; cache hits would make the check a no-op.make VAR=VALdropping variables — GNU make does forward command-line variables to the delegated child; the issue is only the "no default goal" + "ws-fix rename" points.
Full synthesis: see detailed inline comments below.
Replace the hardcoded prefix table with parsing of Taskfile.yml so the list of paths that triggers each CI test target is derived from the test:exp-aitools, test:exp-ssh, and test:pipelines tasks' `sources:`. Also makes the tool CWD-independent via `git rev-parse --show-toplevel`. Co-authored-by: Isaac
Introduce Taskfile.yml as the single source of truth for build, test, lint, format, and codegen commands. Provide a thin Makefile wrapper that forwards all targets to task so existing `make <target>` invocations keep working for single-word targets. Move python/Makefile into the same Taskfile under python:* tasks. Task is vendored via a separate tools/task module to avoid its charmbracelet dependencies conflicting with tools/go.mod (golangci-lint pins older charmbracelet versions). Also update acceptance test helper to build yamlfmt directly — the Makefile wrapper can no longer forward the old `tools/yamlfmt` target since it's now a Task-only dependency. Co-authored-by: Isaac
Replace `make <target>` with `go tool -modfile=tools/task/go.mod task <target>` in CI workflows and the setup-build-environment action. This skips the Makefile wrapper and surfaces task's real target names (e.g. task:exp-aitools, fmt:full) in CI logs. release-build.yml inlines the python wheel build steps (which used to live in python/Makefile) so the release job does not depend on task. Co-authored-by: Isaac
Replace make commands with their task equivalents in the build/test and code-quality sections so AI agents and human readers see the current invocation style. Co-authored-by: Isaac
Each submodule gets its own minimal .golangci.yaml so lint:tools and lint:codegen can run from those directories without inheriting the root config. The root config enables the ruleguard gocritic check against `libs/gorules/rule_*.go`, which golangci-lint resolves relative to the working directory — from inside tools/ or bundle/internal/tf/codegen/ that path does not exist and lint aborts before running. Co-authored-by: Isaac
Shortens `go tool -modfile=tools/task/go.mod task <target>` to `./task <target>` and resolves the modfile via `git rev-parse --show-toplevel` so the script works from any subdirectory. Co-authored-by: Isaac
Replace every `go tool -modfile=tools/task/go.mod task` invocation in CI workflows, the Makefile wrapper, and the dbr:test deco command with the shorter `./task`. The dbr:test command previously pointed at tools/go.mod, which does not include task as a tool — the wrapper fixes that too. Co-authored-by: Isaac
- build: correct generates from `databricks` to `cli` (Go module name) - drop build:vm (unused) - snapshot, snapshot:release, schema:for-docs, docs: add sources/generates - lint:tools, lint:codegen: include .golangci.yaml, go.mod, go.sum in sources - python:docs, python:codegen: include docs and databricks inputs - split fmt into fmt:python, fmt:go, fmt:yaml; fmt and fmt:full dispatch the three in parallel via deps Co-authored-by: Isaac
Co-authored-by: Isaac
Renames lint:check/lint:tools/lint:codegen to lint:go:root/lint:go:tools/ lint:go:codegen to make it explicit they're Go-specific. Adds lint:go that runs all three in parallel so a single invocation covers the whole repo. Co-authored-by: Isaac
The `fmt` aggregate uses the incremental `fmt:go` (via lintdiff.py) for a fast interactive loop, while `fmt:full` uses `fmt:go:full` to sweep the whole tree. Python and YAML tasks are shared between the two aggregates. Co-authored-by: Isaac
The root CLI binary only imports from bundle/, cmd/, experimental/, internal/, libs/, so test-only trees (acceptance/, integration/), the tools/ and bundle/internal/tf/codegen/ submodules, and _test.go files are excluded from build/snapshot/snapshot:release source globs. This lets Task's checksum cache skip rebuilds when only tests change. Task v3 uses `- exclude: <pattern>` for glob exclusion (the `!` prefix from doublestar isn't honored by Task's source matching). Also remove the _build-yamlfmt task — nothing referenced it, and acceptance tests build yamlfmt directly via BuildYamlfmt with exeSuffix for Windows. Co-authored-by: Isaac
Introduce `generate` as an aggregator that runs each generator in sequence: - `generate:commands` (universe + genkit; was top-level `generate`) - `generate:schema` (was `schema`; now has tight sources, no longer gated by `git diff` against merge-base) - `generate:schema-docs` (was `schema:for-docs`) - `generate:docs` (was `docs`) - `generate:validation` (now has tight sources) - `generate:direct` (unchanged) - `generate:openapi-json` (was `codegen:openapi-json`) Each reflection-based generator (schema, schema-docs, validation, docs) lists its Go sources explicitly and includes `go.mod`/`go.sum`, so the aggregator picks up SDK version bumps from `generate:commands` automatically. Test files are excluded from the source globs. Trim `tools/post-generate.sh` to only post-process genkit output (consistency check, tagging.py relocation, whitespace fix). The former `make schema`/`make schema-for-docs`/`make generate-validation`/ `make -C python codegen` calls are gone — those run as standalone tasks in the aggregator now. Update `.github/workflows/push.yml`, `AGENTS.md`, `.agent/rules/auto-generated-files.md`, `.agent/skills/pr-checklist/SKILL.md`, `bundle/docsgen/README.md`, and schema test comments to reference the new task names. Also add `.databricks/` (snapshot binary output) to `.gitignore`. Co-authored-by: Isaac
…efault - Move refschema regeneration into a standalone generate:refschema task with a tight sources list so the cache tracks what actually affects `bundle debug refschema` (cmd, dresources adapters, libs/structs, go.mod). - generate:commands now only runs genkit + post-process. - Aggregator runs generate:refschema between commands and the other generators so fresh out.fields.txt feeds generate:direct. - default now invokes the full generate aggregator instead of just generate:schema + generate:validation. - Fix acceptance/install_terraform.py to skip doc-comment lines when reading ProviderVersion (regressed by the recent doc comment added to bundle/internal/tf/codegen/schema/version.go). Co-authored-by: Isaac
Convention change: the plain task name is the full variant. Quick / incremental variants carry a -q suffix on the top-level namespace. - fmt:full -> fmt, fmt:go:full -> fmt:go (full is the default) - fmt (old incremental) -> fmt-q; fmt:go (old incremental) -> fmt-q:go - lint:full removed; lint is now the aggregator across all Go modules - lint (old incremental root+fix) -> lint-q:go:root; lint-q alias added - lint:go aggregator runs modules sequentially (concurrent golangci-lint invocations have shown unreliable behavior) - default task now runs the -q variants for a quick dev loop - new `all` task runs the full suite (checks, fmt, lint, test, generate, python:lint, python:test) — replaces old default semantics Close the gap where CI only linted the root module: CI now runs `./task lint` which covers root + tools + bundle/internal/tf/codegen. lintdiff.py gains --root-module to skip paths under nested go.mod files; needed for `golangci-lint run` (typechecks) but not for `fmt` (filesystem walk), so only lint-q:go:root opts in. Co-authored-by: Isaac
Drop the --root-module flag; infer from the wrapped subcommand instead. `golangci-lint run` typechecks and needs the filter; `golangci-lint fmt` walks the filesystem and wants to see changed files across modules. Callers no longer need to know which mode applies. Co-authored-by: Isaac
generate:commands is expensive (bazel/genkit build) and requires a universe checkout. Its outputs are committed, so a fresh worktree at any commit already has them. Run `./task generate` explicitly when codegen inputs change. Co-authored-by: Isaac
- Use {{.GO_TOOL}} everywhere (redefine with {{.ROOT_DIR}} so tasks with
`dir:` can reference it).
- Drop dead `{{.X | default ...}}` fallbacks in task-local vars; the
top-level vars are always set and pass through via `deps:` + `vars:`.
- Serialize `checks` (tidy/ws/links) and `test:update-all` — they were
racing on the same files via parallel `deps:`.
- Add `deps: [tidy]` to `snapshot` and `snapshot:release` (matches
`build`).
- Add `desc:` to `generate:direct-apitypes` and
`generate:direct-resources`.
- Expand `lint-q:go:root` desc to note it skips tools/ and codegen/.
- Drop `sources:` from `ws` (script is fast; no point caching).
- Fix code-generation section comment to list all reflection-based
generators (refschema, schema-docs missing).
Co-authored-by: Isaac
- `lint:go:root` and `lint-q:go:root` now exclude tools/** and bundle/internal/tf/codegen/** from sources. golangci-lint `./...` stops at module boundaries, so changes there never affect root-module lint output — no reason to invalidate the cache. - Split `tidy` by module, matching the `lint:go` layout: - `tidy` — aggregator across all three modules - `tidy:root` — root (previously `tidy`) - `tidy:tools` — tools/ - `tidy:codegen` — bundle/internal/tf/codegen `build`, `snapshot`, `snapshot:release` switch to `deps: [tidy:root]` (they only build the root module); `checks` still calls the aggregator. Co-authored-by: Isaac
Mirrors main's e24ae4e (Remove Slow tests #5032): drop test:slow, test:slow-unit, test:slow-acc, and the `-short` flag from test:unit and test:acc. Slow tests support in the acceptance runner was removed because the split between short and fast tests adds complexity and makes it possible to miss failures. Co-authored-by: Isaac
python3 with {{.ROOT_DIR}} produced a malformed path on Windows
(backslashes from ROOT_DIR mixed with forward slash separator).
uv run with a relative path avoids the issue: go-task runs sh: commands
from ROOT_DIR, and uv is already installed in all CI environments.
Co-authored-by: Isaac
uv is not available in all CI environments at the time EMBED_SOURCES is evaluated (e.g. before the setup-uv step in the lint job). Use python with the absolute ROOT_DIR path instead; python is pre-installed on all runner images. Co-authored-by: Isaac
python C:\ROOT_DIR/tools/list_embeds.py produces a mangled path on Windows (backslash+forward-slash combination). Go-task evaluates global sh: vars from ROOT_DIR, so a bare relative path resolves correctly without any path separator issues. Co-authored-by: Isaac
|
.claude/settings.json allowlist still has |
…hing, annotate lintdiff NESTED_MODULES - test-unit-root sources: exclude integration/** and acceptance/** (re-include acceptance/internal/**) to avoid spurious cache invalidations from files outside TEST_PACKAGES - BuildYamlfmt: skip go build when the binary is already newer than tools/go.mod and tools/go.sum, restoring the caching behavior that the old make rule provided - lintdiff.py: add comment clarifying that NESTED_MODULES entries are path prefixes, not exact matches Co-authored-by: Isaac
Co-authored-by: Isaac
Add a build-yamlfmt task to Taskfile.yml (sources: tools/go.mod + go.sum, generates: tools/yamlfmt) so go-task's checksum cache handles staleness. BuildYamlfmt in acceptance_test.go becomes a one-liner. Also add EXE_EXT global var for Windows .exe suffix. Co-authored-by: Isaac
| "Bash(make test-update:*)", | ||
| "Bash(make test-update-templates:*)", | ||
| "Bash(make ws:*)", | ||
| "Bash(./task *)", |
There was a problem hiding this comment.
nit: add task also for folks who have the taskfile CLI installed
The Makefile was a back-compat wrapper kept after #5050 for callers like `make integration`. It silently regressed (`make integration` reported "is up to date" because integration/ is a real directory) and broke the nightly cli-isolated-nightly workflow for ~5 days. Rather than patch the wrapper, drop it entirely and route every caller through ./task directly: - integration/README.md: make integration → ./task integration - experimental/ssh/README.md: make build snapshot-release → ./task ... - tools/bench_parse.py: docstring example updated Note: databricks-eng/eng-dev-ecosystem cli-isolated-tests.yml still calls `make integration|integration-short|dbr-integration`. That repo needs a parallel PR converting those to ./task before this lands, otherwise the nightly will fail with "make: No rule to make target". Co-authored-by: Isaac
Drop the Makefile and route every remaining caller through \`./task\` directly. #5050 introduced \`./task\` and kept the Makefile as a thin shim. eng-dev-ecosystem #1273 was the last external consumer; it now calls \`go tool task\` directly, so the shim has no callers left and can go. The shim was also broken: \`make integration\` was silently exiting 0 because \`integration/\` is a real directory (\`.DEFAULT\` doesn't apply when the target name matches a file/directory). That's how cli-isolated-nightly stopped producing \`output.json\` for ~5 days. ### Diff - Delete \`Makefile\`. - \`integration/README.md\`: \`make integration\` → \`./task integration\`. - \`experimental/ssh/README.md\`: \`make build snapshot-release\` → \`./task build snapshot-release\`. - \`tools/bench_parse.py\`: docstring example. No CI changes needed — \`.github/workflows/\` already calls \`./task\`. This pull request was AI-assisted by Isaac.
…gic there (#5050) Each task describes inputs (sources) and outputs as precise as possible. The tasks mostly match what we had in Makefile but slightly more consistent approach: - All go related tasks (tidy, lint, test) now have 3 subtasks for each submodule (root, tools, codegen) which are aggregated into main one. Previously we did not run tests & linters for tools and codegen modules. - The targets that use lintdiff.py to do incremental run have -q suffix (old fmtfull -> new fmt, old fmt -> new fmt-q). - generate:genkit is task that runs genkit + follow ups. “generate” is aggregate over all generation work. This also consolidates all knowledge about generation in one place: - tools/post-process.sh is removed, the followup commands are now part of generate:genkit task - testmask no longer contains of dependencies of CI targets, it reads those from Taskfile.yml The runner ([go-task](https://github.com/go-task/task)) is packaged as a go tool, no installation need. Shortcut ./task is available to run it. Makefile is temporarily a wrapper that calls ./task. This enables caching - go-task will only re-run if sources changed. This helps when checking after agent work - if they did run linters / tests already, then running ./task is very quick. ``` ~/work/cli-trees/task-file % time ./task … ./task 612.63s user 310.22s system 726% cpu 2:06.95 total ``` ``` ~/work/cli-trees/task-file % time ./task … ./task 2.43s user 10.58s system 133% cpu 9.727 total ``` Additionally, all golangci-lint tasks are fixed to use per-worktree & per-submodule tmp directory, which enables parallel runs within worktree and across worktrees.
Drop the Makefile and route every remaining caller through \`./task\` directly. #5050 introduced \`./task\` and kept the Makefile as a thin shim. eng-dev-ecosystem #1273 was the last external consumer; it now calls \`go tool task\` directly, so the shim has no callers left and can go. The shim was also broken: \`make integration\` was silently exiting 0 because \`integration/\` is a real directory (\`.DEFAULT\` doesn't apply when the target name matches a file/directory). That's how cli-isolated-nightly stopped producing \`output.json\` for ~5 days. ### Diff - Delete \`Makefile\`. - \`integration/README.md\`: \`make integration\` → \`./task integration\`. - \`experimental/ssh/README.md\`: \`make build snapshot-release\` → \`./task build snapshot-release\`. - \`tools/bench_parse.py\`: docstring example. No CI changes needed — \`.github/workflows/\` already calls \`./task\`. This pull request was AI-assisted by Isaac.
Drop the Makefile and route every remaining caller through \`./task\` directly. databricks#5050 introduced \`./task\` and kept the Makefile as a thin shim. eng-dev-ecosystem databricks#1273 was the last external consumer; it now calls \`go tool task\` directly, so the shim has no callers left and can go. The shim was also broken: \`make integration\` was silently exiting 0 because \`integration/\` is a real directory (\`.DEFAULT\` doesn't apply when the target name matches a file/directory). That's how cli-isolated-nightly stopped producing \`output.json\` for ~5 days. ### Diff - Delete \`Makefile\`. - \`integration/README.md\`: \`make integration\` → \`./task integration\`. - \`experimental/ssh/README.md\`: \`make build snapshot-release\` → \`./task build snapshot-release\`. - \`tools/bench_parse.py\`: docstring example. No CI changes needed — \`.github/workflows/\` already calls \`./task\`. This pull request was AI-assisted by Isaac.
Two changes to the four `golangci-lint run` invocations in `Taskfile.yml`: - Add `-j=4` to cap analyzer concurrency. - Add `--allow-parallel-runners` and drop the per-task `TMPDIR` workaround introduced in #5050. `./task lint`, fresh caches, same `tools/go.mod`: | | Cold wall | Cold CPU (user+sys) | Warm wall | |---|---|---|---| | origin/main | 185s | 2282s | 4.1s | | this PR | 76s | 327s | 4.0s | 2.4× faster cold wall, 7× less CPU. Warm runs are cache-bound and unchanged. ## `-j=4` golangci-lint scales poorly past 4–6 workers (upstream [#5149](golangci/golangci-lint#5149)). With three sub-lints running in parallel under `lint-go`, the default `-j=GOMAXPROCS` puts ~48 nominal workers on 16 cores — most of the 2282s on origin/main is kernel scheduling contention. Capping at 4 lets the three modules cooperate. ## `--allow-parallel-runners` By default golangci-lint takes a system-wide lock and refuses to start if another copy is already running. The per-task `TMPDIR` override existed to give each invocation its own copy of that lock file so it didn't collide; `--allow-parallel-runners` tells golangci-lint not to take the lock in the first place. Same outcome, no `TMPDIR` plumbing. Parallel runs happen when `lint-go` fans out to the three module sub-lints, and when multiple worktrees lint at once. Safe to skip the lock because golangci-lint v2.12.0 added content-addressable cache keys ([PR #6445](golangci/golangci-lint#6445)). Before that, two worktrees produced separate cache entries (keyed by absolute path) — no sharing, but also no contention. Now the entries are shared by content, so concurrent runs read and write the same files; the cache uses OS-level file locks with atomic write semantics to keep that safe. `origin/main` already pins v2.12.2. ## Trade-off Two `task lint` runs in the *same* worktree no longer fail-fast with `"parallel golangci-lint is running"` — they race instead, harmlessly but wastefully (duplicate analysis work). Minor. ## Test plan - [x] `task lint` clean - [x] `task lint-q` clean - [x] Parallel `task lint` across two worktrees both succeed - [ ] CI lint job passes This pull request and its description were written by Isaac.
Two changes to the four `golangci-lint run` invocations in `Taskfile.yml`: - Add `-j=4` to cap analyzer concurrency. - Add `--allow-parallel-runners` and drop the per-task `TMPDIR` workaround introduced in #5050. `./task lint`, fresh caches, same `tools/go.mod`: | | Cold wall | Cold CPU (user+sys) | Warm wall | |---|---|---|---| | origin/main | 185s | 2282s | 4.1s | | this PR | 76s | 327s | 4.0s | 2.4× faster cold wall, 7× less CPU. Warm runs are cache-bound and unchanged. ## `-j=4` golangci-lint scales poorly past 4–6 workers (upstream [#5149](golangci/golangci-lint#5149)). With three sub-lints running in parallel under `lint-go`, the default `-j=GOMAXPROCS` puts ~48 nominal workers on 16 cores — most of the 2282s on origin/main is kernel scheduling contention. Capping at 4 lets the three modules cooperate. ## `--allow-parallel-runners` By default golangci-lint takes a system-wide lock and refuses to start if another copy is already running. The per-task `TMPDIR` override existed to give each invocation its own copy of that lock file so it didn't collide; `--allow-parallel-runners` tells golangci-lint not to take the lock in the first place. Same outcome, no `TMPDIR` plumbing. Parallel runs happen when `lint-go` fans out to the three module sub-lints, and when multiple worktrees lint at once. Safe to skip the lock because golangci-lint v2.12.0 added content-addressable cache keys ([PR #6445](golangci/golangci-lint#6445)). Before that, two worktrees produced separate cache entries (keyed by absolute path) — no sharing, but also no contention. Now the entries are shared by content, so concurrent runs read and write the same files; the cache uses OS-level file locks with atomic write semantics to keep that safe. `origin/main` already pins v2.12.2. ## Trade-off Two `task lint` runs in the *same* worktree no longer fail-fast with `"parallel golangci-lint is running"` — they race instead, harmlessly but wastefully (duplicate analysis work). Minor. ## Test plan - [x] `task lint` clean - [x] `task lint-q` clean - [x] Parallel `task lint` across two worktrees both succeed - [ ] CI lint job passes This pull request and its description were written by Isaac.
Each task describes inputs (sources) and outputs as precise as possible.
The tasks mostly match what we had in Makefile but slightly more consistent approach:
This also consolidates all knowledge about generation in one place:
The runner (go-task) is packaged as a go tool, no installation need. Shortcut ./task is available to run it. Makefile is temporarily a wrapper that calls ./task.
This enables caching - go-task will only re-run if sources changed. This helps when checking after agent work - if they did run linters / tests already, then running ./task is very quick.
Additionally, all golangci-lint tasks are fixed to use per-worktree & per-submodule tmp directory, which enables parallel runs within worktree and across worktrees.