Add artifact filter options to bk build download and unit tests - #938
Add artifact filter options to bk build download and unit tests#938lizrabuya wants to merge 6 commits into
bk build download and unit tests#938Conversation
… logic for both bk build download and bk artifacts download
…lls when parameter values are wrong
There was a problem hiding this comment.
One non-blocking concern about validating the new state filter before build resolution. The build-download concurrency rewrite is the part I'd still want a human to sanity-check.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 12124, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
About buildsworth
Model: gpt-5.6-sol with xhigh thinking.
How to request a review: Comment @buildsworth-bk review on the PR, or request buildsworth-bk as a reviewer.
Risk labels (how buildsworth classifies risk) — buildsworth classifies risk itself from the diff. To let it approve, grant L2 approval by mentioning @buildsworth-bk (see L2 approval grant):
- L1 — Low risk (dep bumps, docs/copy, lockfiles, small presentational fixes). buildsworth may approve by default.
- L2 — Standard risk (new UI, additive API fields, refactors). Approved only with an L2 grant; otherwise comment-only.
- L3 — High risk (auth, migrations, payments, secrets, perf-critical paths). Human review always required.
|
|
||
| // Paginate the artifact list up front so every matching artifact gets | ||
| // downloaded, not just the first page. | ||
| artifacts, err := artifact.List(ctx, f.RestAPIClient, bld.Organization, bld.Pipeline, fmt.Sprint(bld.BuildNumber), "", artifactsPath, artifactsState) |
There was a problem hiding this comment.
Non-blocking: artifact.List validates the state, but this call is reached only after build resolution, Builds.Get, and MkdirAll. A typo in --artifacts-state can therefore make network calls and leave an empty build-* directory before returning the validation error. Validating c.ArtifactsState in Run before build resolution would keep invalid input side-effect-free, as the sibling artifact commands do.
There was a problem hiding this comment.
Yes this is fine and intentional. Validation is done on the actual fetching of the artifacts list only and failure should not block the bk build download to download the build's job logs.
Description
This PR addresses a feature gap in
bk build downloadraised in this issue #923 where filtering artifacts by path was missing in the cli.The flags
--artifacts-pathand--artifacts-stateare added tobk build downloadso users can filter artifacts by glob or state instead of downloading everything. This follows the same behaviour recently added in PR #934 .Changes
--artifacts-pathand--artifacts-statethrough to the Buildkite API onbuild downloadartifacts list,artifacts downloadandbuild downloadby creating helper functionsbuild downloadand refactored unit tests forartifacts list/downloadafter creating the helper functionsbuild downloadto still continue with downloading other build filesExamples
Testing
go test ./...)go fmt ./...)Disclosures / Credits
Claude code wrote the unit tests and refactoring to improve any redundant implementations.