ci: let a pull request keep the image it just built by widgetii · Pull Request #135 · OpenIPC/builder

widgetii · 2026-08-27T20:56:38Z

Stage artifacts and Upload build artifacts were gated on
github.event_name != 'pull_request', sharing that guard with Send binary and
the publish job. Those two need it — one uses the Telegram bot secret, the
other writes shared release assets. The upload does not:
actions/upload-artifact is run-scoped and touches no secret and no shared
mutable state.

The effect is that a pull request compiles a complete firmware image and then
discards it. Stage artifacts, Upload build artifacts, Send binary and
Publish releases all report skipped on a green PR run, and the artifact
count is zero. A contributor adding a device profile therefore cannot obtain the
image their own PR just built — the only route to a flashable file is asking a
maintainer to dispatch build-one for them.

#119 hit exactly this. The author planned to "flash the CI artifact once this
branch produces one", which would never have resolved; I dispatched build-one
by hand to unblock it.

Why this is safe

publish carries its own github.event_name != 'pull_request' in its if:,
independently of these steps:

if: >-
  !cancelled() &&
  github.event_name != 'pull_request' &&
  needs.preflight.outputs.should_build == 'true' &&
  contains(fromJSON('["success", "failure"]'), needs.buildroot.result)

So producing fw-* on a pull request cannot reach a release. It only makes the
image downloadable from the run that built it. Send binary keeps its guard
too, so no PR touches the Telegram secret or the dev channel.

What it costs

select narrows the matrix via ci-matrix.py, and the cost depends on what the
PR touches — measured, not estimated:

PR shape	devices built	of 96
device profile (#119)	1	1%
CI infrastructure (this PR)	15	16%

So the case this change exists for — a contributor adding a device — produces a
single ~7 MB artifact. A workflow change like this one produces 15, roughly
100 MB at 7-day retention. Both are modest, but the second is not "one image per
PR" and I would rather state it than round it down.

Retention, per event

retention-days: ${{ github.event_name == 'pull_request' && 7 || 1 }}

A nightly's artifact is consumed by publish inside the same run and nobody
downloads it afterwards, so a nightly gains nothing from a longer window — and
since it builds the whole matrix daily, a week of retention would keep seven
full matrices in storage instead of one, for no reader. A PR builds a handful
and has a person waiting on them, so one day is too short there.

The first revision of this PR set 7 unconditionally, which contradicted that
reasoning; corrected in b14cc52 after review.

Verification

The change is two if: simplifications and one retention bump; the guards that
matter are untouched. .github/scripts/lint-workflow-shell.py passes — 31 run
blocks, all clean.

The honest test is this PR's own run: Upload build artifacts should now
succeed and attach fw-*, while Publish releases stays skipped. If both hold
here, the change does what it claims. Worth confirming that before merge rather
than taking the reasoning for it.

Stage artifacts and Upload build artifacts were gated on `github.event_name != 'pull_request'`, alongside Send binary and the publish job. Those two need the guard -- one uses the Telegram bot secret, the other writes shared release assets. The upload does not: actions/upload-artifact is run-scoped and touches no secret and no shared state. The effect was that a PR compiled a complete firmware image and then threw it away. A contributor adding a device profile could not obtain the image their own PR had just built, so the only route to a flashable file was asking a maintainer to dispatch build-one. builder#119 hit exactly this: the author planned to "flash the CI artifact once this branch produces one", which would never have resolved. Producing fw-* on a pull request cannot reach a release, because publish carries its own event_name guard independently of these steps. And the matrix is narrowed to affected devices by ci-matrix.py, so this is on the order of one image per PR rather than the full device set. retention-days 1 -> 7: a nightly's artifact is consumed by publish in the same run and never needs to outlive it, while one day is too short a window for a contributor in another timezone to act on.

qodo-free-for-open-source-projects · 2026-08-27T20:57:12Z

PR Summary by Qodo

Retain pull request firmware artifacts for contributor testing

🐞 Bug fix ✨ Enhancement ⚙️ Configuration changes 🕐 Less than 10 minutes

AI Description

• Preserve firmware images produced by pull request builds.
• Keep secret-backed messaging and shared release publishing blocked for pull requests.
• Extend artifact retention to seven days for practical contributor access.

Diagram

graph TD
  PR["Pull Request"] --> Select["Device Selection"] --> Build["Firmware Build"] --> Stage["Stage Artifacts"] --> Upload["Run Artifact"] --> Gate{"Pull Request?"}
  Gate -->|Yes| Skip["Skip Publishing"]
  Gate -->|No| Publish["Publish Releases"]

High-Level Assessment

The current approach is appropriate: run-scoped GitHub artifacts provide contributor access without secrets or shared mutable state, while existing guards continue protecting Telegram delivery and release publication. Maintainer-dispatched builds or PR-specific releases would add friction or unnecessary shared-state risk.

Files changed (1) +16 / -7

Other (1) +16 / -7

master.ymlUpload pull request firmware artifacts for seven days +16/-7

Upload pull request firmware artifacts for seven days

• Removes the pull request exclusion from artifact staging and upload steps, allowing contributors to download firmware built by their own PRs. Retains the independent pull request guards on Telegram delivery and release publishing, and increases artifact retention from one to seven days.

.github/workflows/master.yml

qodo-free-for-open-source-projects · 2026-08-27T20:57:52Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. ~~Nightly storage grows sevenfold~~ ✓ Resolved 🐞 Bug ➹ Performance

Description

Setting retention-days: 7 unconditionally retains every full nightly matrix artifact for seven
days, even though only pull-request artifacts need the longer download window. Since non-PR events
build the entire roughly 107-device matrix daily, this changes steady-state nightly artifact storage
from about one full matrix to seven.

Code

.github/workflows/master.yml[317]
+          retention-days: 7

Evidence

The workflow runs on a daily schedule, and its own matrix documentation says every event except
pull_request receives the full device matrix; the workflow also identifies that full set as 107
devices. Each successful matrix entry uploads an artifact, so changing the shared upload step from
one-day to seven-day retention retains several additional complete nightly matrices rather than only
extending PR artifact availability.

.github/workflows/master.yml[3-15]
.github/workflows/master.yml[118-123]
.github/workflows/master.yml[297-318]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The seven-day retention period is applied to all artifact uploads, multiplying storage for daily full-matrix nightlies even though those artifacts are consumed in the same run.
## Issue Context
Keep seven-day retention for pull-request artifacts, but preserve the shorter retention for scheduled and dispatched release builds.
## Fix Focus Areas
- .github/workflows/master.yml[311-318]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Tip of the day

💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Addresses the review finding on this PR. The previous commit set retention-days: 7 unconditionally, which was inconsistent with its own stated rationale: a nightly's artifact is consumed by `publish` inside the same run and nobody downloads it afterwards, so a nightly has no use for the longer window -- and the nightly builds the whole device matrix daily, so a week of retention keeps seven full matrices in storage instead of one, for no reader. Make it per-event, which is what the reasoning argued for: retention-days: ${{ github.event_name == 'pull_request' && 7 || 1 }} pull_request gets 7, every other event keeps the existing 1.

widgetii · 2026-08-27T21:03:32Z

Fixed in b14cc52 — the finding was correct, and it caught a real inconsistency
rather than a style point.

The rationale I wrote for the change argued that a nightly's artifact is consumed
by publish inside the same run and never needs to outlive it. Having argued
that, I then set retention-days: 7 unconditionally, which applies the longer
window to precisely the case that has no use for it — and since a nightly builds
the entire device matrix daily, that is seven full matrices held in storage
instead of one, for no reader at all. The code contradicted its own comment.

Now per-event:

retention-days: ${{ github.event_name == 'pull_request' && 7 || 1 }}

pull_request → 7, every other event → 1, unchanged from today's behaviour.
Checked the expression against the A && B || C idiom for each trigger the
workflow declares:

event	retention-days
`pull_request`	7
`schedule`	1
`workflow_dispatch`	1
`push`	1

.github/scripts/lint-workflow-shell.py still passes.

Also worth recording that the first revision already demonstrated the change
works: on the run for 7530413, the first device to finish reported
Stage artifacts success, Upload build artifacts success, and
Send binary skipped, with Publish releases not running. That is the
behaviour this PR claims — the image is kept, and nothing secret-backed or
release-writing is reached from a pull request.

widgetii requested review from cronyx, flyrouter and viktorxda as code owners August 27, 2026 20:56

qodo-free-for-open-source-projects Bot reviewed Aug 27, 2026

View reviewed changes

Comment thread .github/workflows/master.yml Outdated

widgetii merged commit 832c2af into master Aug 27, 2026
20 checks passed

widgetii mentioned this pull request Aug 28, 2026

Add gk7202v300_lite_generic-w7 device profile (GK-W7, GC2053-DVP, SSV6006C Wi-Fi) #119

Open

Navigation Menu

Uh oh!

Uh oh!

ci: let a pull request keep the image it just built - #135

ci: let a pull request keep the image it just built#135
widgetii merged 2 commits into
masterfrom
ci/pr-build-artifacts

widgetii commented Aug 27, 2026 •

edited

Loading

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 27, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 27, 2026 •

edited

Loading

Uh oh!

Uh oh!

widgetii commented Aug 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

widgetii commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this is safe

What it costs

Retention, per event

Verification

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 27, 2026

PR Summary by Qodo

Uh oh!

qodo-free-for-open-source-projects Bot commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

widgetii commented Aug 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

widgetii commented Aug 27, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented Aug 27, 2026 •

edited

Loading