feat(apply): add --since/--force for git-history-based deletion sync - #253
Open
mmanciop wants to merge 12 commits into
Open
feat(apply): add --since/--force for git-history-based deletion sync#253mmanciop wants to merge 12 commits into
mmanciop wants to merge 12 commits into
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.
5 tasks
| // internal/git/snapshot.go's ingestDocuments — into the base file path and | ||
| // the document's 0-based index within it, matching parseMultiDocumentYAML's | ||
| // return-slice indexing. | ||
| func splitMultiDocPath(path string) (basePath string, docIndex int) { |
Contributor
There was a problem hiding this comment.
question: Should we have it in the api client? What happens if in TF we have a yaml file with many definitions inside?
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
apply --since <ref>andapply --force, both experimental: deletes assets whose definition existed at<ref>(viainternal/git, a thinos/execwrapper over git plumbing) but is no longer present in-f's current contents, matched by identifier (id/origin), never by file path — including PrometheusRule's CRD-shared-identifier vs. per-alert-name distinction.--dry-run --sincepreviews the deletion plan alongside the existing create/update preview, merged into one per-file listing sorted by identifier; a deleted asset's display name is resolved from git history rather than left as a bare id.--dry-run:[{"path": ..., "changes": [{"op": "apply"|"delete", "name": ..., "originOrId": ...}]}].test/e2etier (testcontainers-go): the realdash0binary invoking the realgitbinary across a process boundary, since--sinceshells out to git rather than using a Go git library.test/roundtrip/test_apply_since_{roundtrip,idempotency,ref_edge_cases}.sh, run and passing against a real Dash0 dev environment.--since/--forceend to end:docs/commands.md,README.md, the Docker/gitlimitation, the GitHub Actions invocation pattern, theinternal/skillagent-skill bundle, and a new "flag-level promotion" section indocs/promoting-commands-to-stable.md(this is the first flag-level experimental gate in the CLI).openspec/changes/add-diff-and-since-flag/) and the follow-onasset-synchGitHub Action proposal (not yet implemented).Both
--sinceand--forceremain gated behind--experimental/-X.dash0 diff(a separate preview command planned for this same change) is not part of this PR — see Section 5 of the tasks doc.Found while testing / writing docs
PrometheusRuleCRD with 2+ alerting rules sharing onedash0.com/idnever produces more than one live check rule via ordinarycreate/apply— each alert's PUT overwrites the previous one under the shared id. Predates this branch (root cause is in the siblingdash0-api-client-go's CRD-to-check-rule conversion) but means--since's alerting-rule-partial-removal scenario (1.6/4.7 in the tasks doc) has no real-world-reachable roundtrip coverage — only the mock-server-based unit/integration tests exercise it.--dry-run --sincebug where a-ftarget that's a subdirectory of the repo (not the repo root) grouped a deletion under its repo-root-relative path while that file's surviving documents grouped under their-f-target-relative path — inconsistent prefixing in the same listing. Fixed, with regression tests.docs/commands.md's plainapply -f assets/example: real output prefixes each line with the path relative to-f's own target, not the invoking shell's directory (dashboard.yaml: ..., notassets/dashboard.yaml: ...). Fixed alongside the--sincedocs.Known follow-ups (tracked in
openspec/changes/add-diff-and-since-flag/tasks.md)dash0-api-client-gochange and drop thego.modreplace directive it's currently consumed through — CI will not go green until this lands, since the replace directive points at a local sibling checkout.dash0 diffitself (Section 5) — not part of this PR.Test plan
make buildmake test(unit + integration)make lintmake skill-validatetest_apply_since_roundtrip.sh,test_apply_since_idempotency.sh,test_apply_since_ref_edge_cases.sh— run individually against a real Dash0 dev environment, all pass, all cleaned upmake test-e2e— not run in this session (requires Docker)make test-roundtrip(every asset type, not just--since's new scripts) — not run in this session