feat(time-series-aggregations): add the time-series-aggregations command group - #281
feat(time-series-aggregations): add the time-series-aggregations command group#281jochil wants to merge 12 commits into
Conversation
495e1ce to
2e64850
Compare
This brings in the time series aggregation client. The Nix vendorHash changes in the same commit, because it has to change whenever go.sum does.
…ands Every `update` command carried its own copy of the same logic: take the id from the argument or from the file, and reject an argument that matches neither. The copies had drifted, so the same mistake produced a different message depending on which asset kind you hit it with. One helper in internal/asset now answers that for all of them. The wording is the same everywhere, and it names only the identifiers the file really carries.
Both `create` and `apply` call this helper, so the two paths cannot drift apart. Three behaviors are specific to this asset kind. The origin label is required and is the only upsert key, so every write is a PUT to the origin and the CLI never sends a POST. The diff clears the id, which no hand-written file ever carries. And a reused origin gets an explanation, because the API answers a cross-dataset collision with a bare 400 that names neither constraint.
Adds `dash0 time-series-aggregations` (alias `tsa`) with the five standard subcommands. It is dataset-scoped, and every endpoint needs the org admin role.
A document without the origin label fails during validation, before the run applies anything, because there is no id to fall back to.
…ests Both scripts check for the admin role first and skip cleanly without it, so they still pass on an ordinary token.
…e skill topic This also corrects two existing docs. The idempotent-delete rule claimed that a missing asset errors without `--force`. It does not, because those endpoints answer 2xx. And the identifier table linked to a heading that does not exist.
2e64850 to
68b6d00
Compare
| fmt.Printf("Dataset: %s\n", dash0api.GetTimeSeriesAggregationDataset(aggregation)) | ||
| fmt.Printf("Origin: %s\n", origin(aggregation)) | ||
| fmt.Printf("Interval: %s\n", interval(aggregation)) | ||
| fmt.Printf("Enabled: %t\n", aggregation.Spec.Enabled) |
There was a problem hiding this comment.
issue: GetTimeSeriesAggregation returns (nil, nil) on a 200 with a non-JSON Content-Type, so this panics. Every other accessor in the package guards it.
There was a problem hiding this comment.
runGet now returns an error when the client hands back a nil aggregation. I also added a regression test for this
| var flags asset.FileInputFlags | ||
|
|
||
| cmd := &cobra.Command{ | ||
| Use: "update [origin-or-id] -f <file>", |
There was a problem hiding this comment.
issue: <origin-or-id> only works for a file exported via tsa get -o yaml. Origin is mandatory and ids are server-assigned, so ResolveUpdateKey takes the Origin != "" && ID == "" branch and rejects an id copied from tsa list. Long states the real rule; Use and the example at line 32 don't.
There was a problem hiding this comment.
good catch. Updated Use and Long
| @@ -692,7 +692,7 @@ $ echo $? | |||
| ``` | |||
|
|
|||
| Without `--force`, a missing asset surfaces as a non-zero exit with a "not found" error. | |||
There was a problem hiding this comment.
issue: contradicts the note this PR adds to cli-naming-conventions.md. This file is the copy synced to dash0.com/docs.
There was a problem hiding this comment.
I corrected this in cli-naming-conventions.md
| # Origins are unique per organization while each aggregation belongs to one | ||
| # dataset, so this is the error a user hits the first time they point one | ||
| # asset directory at a second dataset. | ||
| OTHER_DATASET="${DASH0_TSA_OTHER_DATASET:-default}" |
There was a problem hiding this comment.
suggestion (non-blocking): if the API accepts the cross-dataset apply instead of returning 400, cleanup only deletes from the active dataset and the aggregation leaks into $OTHER_DATASET. Delete in both, or require DASH0_TSA_OTHER_DATASET to be set explicitly.
There was a problem hiding this comment.
Fixed in a new commit by deleting from both datasets in cleanup. I went with that rather than requiring DASH0_TSA_OTHER_DATASET, since that would turn step 5 from "skips politely" into "fails unless configured" for anyone running the suite, CI included. More than happy to switch if you'd prefer it strict.
…n an undecodable 200 A 200 whose body the client cannot decode as JSON leaves the aggregation nil with a nil error, and the summary path reads aggregation.Spec directly rather than through the nil-tolerant getters, so `tsa get` panicked. The regression test needs the mock server to answer with a non-JSON content type, so MockResponse grows an optional ContentType. It keeps defaulting to application/json.
`Use` and the example offered `<origin-or-id>`, but an id only resolves when the document carries that same id. Origins are mandatory and ids are server-assigned, so a hand-written document has only an origin, and an id copied from `tsa list` is rejected. The help now says origin, and `Long` explains when an id also works.
commands.md still said a missing asset exits non-zero without `--force`, which contradicts the note this branch adds to cli-naming-conventions.md. Most delete endpoints answer 2xx, so `delete <typo>` exits 0 either way. This file is the copy synced to dash0.com/docs, so the wrong version was the published one.
Step 5 expects the cross-dataset apply to be rejected. If the API ever accepts it, the aggregation lands in the other dataset and the old cleanup could not reach it, leaking an origin that the whole organization can see.
Same facts, fewer words, and no comment that only restates the name of the thing below it. Test doc comments now say why the case exists instead of repeating the test name. 348 comment lines down to 288.
2 participants
Adds
dash0 time-series-aggregations(aliastsa) with the five standard CRUD subcommands, pluskind: Dash0TimeSeriesAggregationinapplyandapply --since.Closes #280. CLI slice of SC-481 (SC-600).
Three things that differ from other asset kinds
Two doc fixes found along the way
The idempotent-delete rule claimed that a missing asset errors without
--force. It does not.dashboards,check-rules,views, andspam-filtersall exit 0 for an id that does not exist, because those endpoints answer 2xx instead of 404. The rule now says so.The identifier table also linked to a heading that does not exist.
Also in here
Two refactors that came out of reviewing the above.
The cross-dataset advice now goes through the
\nHint:convention, so agent mode shows it in thehintfield instead of leaving that field empty.All nine
updatecommands now share one update-key resolver, replacing nine copies that had drifted apart. Same wording everywhere, and it names only the identifiers the file really carries.One thing #280 asks for that this does not do
#280 wants
tsa delete <missing>to exit non-zero with a clean "not found". That needs a preflight GET, and I left it out deliberately.I checked the four sibling endpoints first. Deleting a missing id without
--forceexits 0 ondashboards,check-rules,views, andspam-filters, because they answer 2xx. Adding a preflight here would make time series aggregations the only asset type that errors on a missing id, which is a worse inconsistency than the one it fixes.IsAlreadyDeletedstays wired, so this is already right if the API starts returning 404.Everything else in #280 is done.
Verification
make lint,make test-unit, andmake test-integrationall pass, and every commit in the branch builds on its own.Tested live against dash0-development with an admin token: create, reapply as a no-op, list, get,
get -o yamlthen apply as a no-op, a changed interval showing the right diff, the cross-dataset hint, and delete. A 58-case manual pass came back with no failures. Both roundtrip scripts pass with an admin token and skip without one.