Skip to content

Add artifact filter options to bk build download and unit tests - #938

Open
lizrabuya wants to merge 6 commits into
mainfrom
feature/add-artifact-filter-on-build-download
Open

Add artifact filter options to bk build download and unit tests#938
lizrabuya wants to merge 6 commits into
mainfrom
feature/add-artifact-filter-on-build-download

Conversation

@lizrabuya

@lizrabuya lizrabuya commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Description

This PR addresses a feature gap in bk build download raised in this issue #923 where filtering artifacts by path was missing in the cli.

The flags --artifacts-path and --artifacts-state are added to bk build download so users can filter artifacts by glob or state instead of downloading everything. This follows the same behaviour recently added in PR #934 .

Changes

  • Wire --artifacts-path and --artifacts-state through to the Buildkite API on build download
  • Combined list artifacts logic used in artifacts list, artifacts download and build download by creating helper functions
  • Add unit tests for build download and refactored unit tests for artifacts list/download after creating the helper functions
  • Explicitly add warnings when no artifacts matches path/state filters for build download to still continue with downloading other build files

Examples

   $ bk build download --pipeline my-pipeline --artifacts-path "log/rspec*.json"
   $ bk build download --pipeline my-pipeline --artifacts-state finished

Testing

  • Tests have run locally (with go test ./...)
  • Code is formatted (with go fmt ./...)

Disclosures / Credits

Claude code wrote the unit tests and refactoring to improve any redundant implementations.

@lizrabuya
lizrabuya marked this pull request as ready for review August 5, 2026 10:04
@lizrabuya
lizrabuya requested review from a team as code owners August 5, 2026 10:04

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cmd/build/download.go

// 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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant