Repository navigation
chore: sync development into main - #16
nicomiguelino wants to merge 3 commits into
Conversation
* chore(deps): bump @screenly/edge-apps to 1.2.1 Removes the screen name from the app header (Screenly/edge-apps-library#56). * chore: regenerate screenshots for app-header fix Screenshot regeneration failed on the previous commit due to memory pressure on the dev machine. Re-ran cleanly and confirmed the header no longer shows the screen name.
- Set STAGE_EDGE_APP_ID and PRODUCTION_EDGE_APP_ID repo variables and pass them through to the initialize/update actions as edge_app_id - Drop id from screenly.yml and delete screenly_qc.yml since a single manifest is now used regardless of environment - Pin edge-apps-actions to a commit for now, since v26.9.0 can't be tagged there until a repo ruleset is adjusted
GitHub Actions
Failure
There was a problem hiding this comment.
🔵 Needs a closer look
It changes the deployment workflows and introduces a large dependency/lockfile update (toolchain changes) that should be validated by a human reviewer with CI/deploy context.
Pull request overview
This PR syncs the @screenly/edge-apps dependency bump from development into main, and adjusts manifests/workflows to align with the updated Edge Apps tooling and deployment configuration.
Changes:
- Bumps
@screenly/edge-appsfrom^1.0.0to^1.2.1and updatesbun.lockaccordingly. - Removes the
idfromscreenly.ymland deletesscreenly_qc.yml(QC manifest). - Pins Screenly GitHub Actions to a specific commit SHA and passes
edge_app_idexplicitly via secrets/vars.
File summaries
| File | Description |
|---|---|
screenly.yml |
Removes the embedded app id from the manifest. |
screenly_qc.yml |
Deletes the QC manifest file entirely. |
package.json |
Updates @screenly/edge-apps version to ^1.2.1. |
bun.lock |
Updates resolved dependency graph to match the new @screenly/edge-apps version. |
.github/workflows/update-edge-app.yml |
Pins the update action to a commit SHA and provides edge_app_id via secrets/vars. |
.github/workflows/initialize-edge-app.yml |
Pins the initialize action to a commit SHA and provides edge_app_id via secrets/vars with environment-based selection. |
Review details
- Files reviewed: 5/26 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Mirrors Screenly/airtable-app#16: pin edge-apps-actions' initialize/update to the commit that adds the edge_app_id input (f4ccd2b, ahead of its next tagged release) and pass it through from an EDGE_APP_ID secret, falling back to STAGE_EDGE_APP_ID / PRODUCTION_EDGE_APP_ID repo variables per environment. Backward compatible: with none of those set, edge_app_id resolves to an empty string and both workflows behave exactly as before (manifest id lookup, branched between screenly.yml/screenly_qc.yml). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Heads up: this PR currently has merge conflicts with Flagged by an automated PR-hygiene sweep — no action needed beyond the rebase, and no reply expected. |
GitHub Actions
Waiting
vpetersson-bot
left a comment
vpetersson-bot
left a comment
There was a problem hiding this comment.
Flagging this because the description and the diff don't match, and the gap is on the riskier side.
The body says this syncs the @screenly/edge-apps 1.2.1 bump. The diff also does three other things, none of them mentioned:
1. actions/checkout is downgraded from v7 to v6 — but only in update-edge-app.yml. initialize-edge-app.yml keeps @v7. So main would end up with two workflows in the same repo disagreeing about checkout, and the one that deploys to production is the one going backwards. If that's deliberate (a v7 incompatibility with the pinned action?) it needs a comment saying so; if it's a stray revert picked up in the sync, it should be fixed on development before this lands.
2. The app's id moves out of screenly.yml and into CI configuration. screenly.yml loses its id: line, and the workflows gain edge_app_id: ${{ secrets.EDGE_APP_ID || vars.PRODUCTION_EDGE_APP_ID }} (and the stage equivalent). That's a meaningful operational change: app identity is no longer in the repo, and deploys now depend on repository variables being set correctly per environment. If PRODUCTION_EDGE_APP_ID is unset or wrong, the production job either fails or — worse — targets the wrong app. Please confirm both variables are populated in this repo's environments before merging. This is the part I'd most want checked.
3. Pinning Screenly/edge-apps-actions from @v1 to a commit SHA is a genuine improvement and worth calling out as intentional rather than leaving it to be discovered — a moving @v1 tag in a workflow holding SCREENLY_API_TOKEN is exactly what SHA-pinning is for.
None of this is necessarily wrong — it all presumably landed on development deliberately. But a development → main sync is the moment it reaches production deploys, and a one-line description that mentions only a dependency bump means nobody reviews the parts that matter. Worth expanding the body to list what's actually crossing over.
4. screenly_qc.yml is deleted outright — 74 lines, including its own app id and the full settings schema for the QC app. Completely unmentioned in the description. If the QC app is retired, fine, but that deserves a sentence; if it's still in use anywhere, deleting its manifest from main is not something to carry in on a dependency-bump sync.
(Reviewed alongside the matching sync in rss-reader-app — points 1-3 apply there too.)
Syncs the @screenly/edge-apps 1.2.1 dependency bump (removes the screen name from the app header) from development into main.