refactor(apply): move document model to internal/asset - #255
Open
mmanciop wants to merge 13 commits into
Open
Conversation
KindDisplayName had no case for the normalized kind "team", so any caller relying on it for a team asset printed the raw lowercase "team" instead of a proper display name -- inconsistent with every other kind, whose display name is a distinct, human-readable string.
…mmand RequireExperimentalFlag lets a stable command keep one flag behind --experimental/-X, for cases where the flag itself needs more time to prove out before the rest of the command's behavior is committed to.
FindNonHiddenYAMLFiles centralizes the hidden-file/YAML-extension filtering apply's directory scan already did, as a fs.WalkDirFunc callers pass to their own filepath.WalkDir call. Kept as a walk-callback rather than a self-contained walker so the WalkDir invocation stays visible at each call site.
Adds internal/git, a thin wrapper over git plumbing (rev-parse, merge-base, cat-file, ls-tree) that classifies a --since ref (empty, all-zeros sentinel, resolved ancestor, resolved non-ancestor, unresolvable) and diffs asset identifiers between that ref and the current -f contents to compute a deletion plan -- by identity (id/origin), never by file path, including PrometheusRule's CRD-shared-identifier / per-alert-name distinction. apply --since wires this into the existing create/update flow: ref-resolution errors, a confirmation prompt for a non-ancestor ref (bypassable with the new --force), per-asset deletion confirmation, and a non-zero exit when a deletion is declined. Both are gated behind --experimental/-X for now (--since via the new RequireExperimentalFlag), pending real-world testing before promotion. Includes the openspec proposal/design/specs/tasks for this change and the follow-on asset-synch GitHub Action, and a go.mod replace pointing at a local dash0-api-client-go checkout for the identifier-extraction helpers (ExtractIdentifier/ExtractPrometheusAlertNames) this depends on, pending their release.
… e2e harness Replaces the originally-planned zipped-repo fixture design with a declarative GitRepoFixture YAML format (internal/testutil/gitscenario.go) describing commit history directly -- readable and diffable in review, with no separate generation step. Validated against a new JSON Schema (git_repo_fixture.schema.json) in gitscenario_test.go. Adds the testcontainers-go-based end-to-end tier (test/e2e) that runs the real dash0 binary against a real git binary inside a container, the one gap unit and integration tests can't cover for --since, which shells out to git rather than using a Go git library. Wired into a new `make test-e2e` target and CI job. Renames internal/testutil/fixtures/apply's readme.txt placeholders to .gitkeep with explanatory comments, matching the project's convention.
Adds four checklist items for gaps a ce-doc-review pass found in the --since implementation: releasing the pending dash0-api-client-go dependency and dropping the go.mod replace directive (1.9), the check-rule name-collision risk in the alerting-rule deletion lookup (4.16), hardening the ID-only spam-filter deletion path to fail instead of warn (4.17), and documenting dash0 diff's exit-code CI-consumption pattern once that command exists (8.8). Tracking only; no code changes.
Merges --dry-run's validation preview with --since's deletion preview
into one per-file listing (previously two separately-headed sections
that repeated a file's path when it had both a surviving and a removed
document), sorted by id/origin within each file. Every line now reads
"Apply|Delete <Kind> "<name>" (<id>)", including for a deleted asset --
its name is resolved by re-reading the asset's content from git
history at --since's ref (internal/git's ReadFileAtRef), falling back
to a "<name>" placeholder only if that lookup fails. Extends the same
name lookup to the real (non-dry-run) per-asset deletion confirmation
prompt and success/decline messages.
Adds agent-mode JSON output for --dry-run: an array of
{path, changes: [{op, name, originOrId}]}, covering the plain,
--since-merged, and single-file-target cases uniformly. Factors the
row-building/grouping logic previously duplicated between apply.go and
since.go into a shared buildDryRunRows so text and JSON rendering
cannot drift from each other.
…ironment Adds three roundtrip scripts run against a real Dash0 environment, closing the one test tier --since previously only had mock-server/e2e-container coverage for: whole-file and multi-document partial deletion, apply --since idempotency (second run against the new baseline reports no changes), and the all-zeros-sentinel / non-ancestor-ref edge cases (including the --force confirmation bypass). Registers all three in run_all.sh's API_TESTS list. Deliberately does not cover PrometheusRule alerting-rule partial removal: verified against the real API that a CRD with 2+ alerts sharing one dash0.com/id never produces more than one live check rule via create/apply (each alert's PUT overwrites the previous one under the shared id) -- filed as #254, since it's a pre-existing bug in the sibling dash0-api-client-go's CRD conversion, not something --since introduced, but it does mean that scenario has no real-world-reachable coverage beyond the existing mock-server-based tests.
…bdirectory -f target
gitutil.Deletion.Path (from git ls-tree) is always repo-root-relative,
while assetDocument.filePath is always relative to the -f target itself.
When -f points at a subdirectory of the repo rather than the repo root,
these two bases diverge: a deletion candidate's path (e.g.
"dashboards/removed.yaml") no longer matches its file's surviving
documents' path ("keep.yaml"), so the merged --dry-run listing grouped
them under different, inconsistently-prefixed entries instead of one
entry per file.
Threads the --since target's scope (already computed in
computeDeletionPlan for the git-side pathspec) through to the dry-run
renderer, stripping it from each deletion path before grouping so both
sides use the same basis. Found while writing documentation examples for
apply --since and noticing the discrepancy against real output.
apply --since/--force shipped in this branch with no documentation at all beyond an incidental spam-filter note. Adds: - docs/commands.md: --since/--force in the apply flags table and usage line, a dedicated `apply --since` (experimental) subsection covering identity/deletion semantics, the merged --dry-run preview (including its agent-mode JSON shape), per-asset confirmation and --force, the non-zero exit on a declined deletion, the ref-resolution error messages (empty/all-zeros/unresolvable), the non-ancestor warning, the no-identifier hard-fail, PrometheusRule alerting-rule deletion, the git/Docker requirement, and a GitHub Actions invocation example; plus a "Common workflows" entry and a taxonomy-intro mention. - README.md: a --since example in "Applying assets", cross-referencing the full reference. - docs/installation.md + README.md: a note that the Docker image (built FROM scratch) has no git, so --since is unavailable from it. - docs/promoting-commands-to-stable.md: a new "Flag-level promotion" section (apply --since is the first flag-level experimental gate in this CLI, so the existing whole-command guide didn't cover it). - internal/skill/gen bundle regenerated (make skill-bundle) to pick up the docs/commands.md changes; SKILL.md's apply summary and workflow list hand-updated to match. Also fixes a pre-existing inaccuracy noticed while writing these examples: apply's per-file output prefixes each line with the file's path relative to -f's own target, not the invoking shell's directory, so `dash0 apply -f assets/` prints "dashboard.yaml: ...", not "assets/dashboard.yaml: ...". Marks tasks.md's Section 8 items done for the --since/--force scope; the diff-specific portions of 8.1/8.2/8.3/8.6 and all of 8.8 remain open until dash0 diff (Section 5) exists.
Extract assetDocument and its reading/parsing helpers (readMultiDocumentYAML, parseMultiDocumentYAML, readDirectory, discoverFiles, parseDocumentHeader, formatNameAndId, pluralize) out of internal/apply into internal/asset as exported Document/ReadMultiDocumentYAML/etc. apply is the only current caller, but the upcoming diff command needs the same document discovery and parsing without apply's write-path baggage, and project convention already places logic shared across apply and per-asset commands in internal/asset.
9 tasks
mmanciop
force-pushed
the
feat/sync-action
branch
from
August 25, 2026 11:19
3cad860 to
4d38819
Compare
nicolastakashi
force-pushed
the
feat/sync-action
branch
2 times, most recently
from
August 27, 2026 12:41
f053c10 to
a81d3ea
Compare
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
assetDocumentand its reading/parsing helpers (readMultiDocumentYAML,parseMultiDocumentYAML,readDirectory,discoverFiles,parseDocumentHeader,formatNameAndId,pluralize) out ofinternal/applyintointernal/assetas exportedDocument/ReadMultiDocumentYAML/etc.applyis updated to use the new exported types/functions, and the corresponding tests moved tointernal/asset/document_test.go.dash0 diffcommand (stacked on top of this branch), which needs the same document discovery/parsing but must not depend onapply's write-path logic.Why this is stacked
This PR targets
feat/sync-action(notmain), since it builds on theapply --since/--forcework in #253. Thediffcommand PR will stack on top of this one.Test plan
go build ./...go vet ./...go test ./...go test -tags=integration ./...make lint