OpsDeck/mobile
Watch
2
0
Fork
You've already forked mobile
 0
Code Issues 1 Pull requests Projects Releases Packages Wiki Activity Actions

Follow-ups from the #6 review: PR-check cost, the LF story for existing trees, spotless scope, stale lint baseline #8

New issue
Closed
opened 2026-08-24 16:35:02 +02:00 by julian · 1 comment
julian commented 2026-08-24 16:35:02 +02:00
Owner
Copy link

#6 landed the spotless import-order rule and the PR gate that enforces it. The approving review left a set of non-blocking notes; this is where they live so they don't evaporate with the merge.

Grouped by whether they are a code change, a repo setting, or a thing we only need to know.

Code changes

1. pr-checks.yml has no concurrency and no types:.
Three pushes to one PR start three full cold runs, in parallel, on the single self-hosted runner. A cold run costs ~4m30s. Cheap version:

on:
  pull_request:
    types: [opened, synchronize, reopened]

concurrency:
  group: pr-${{ github.ref }}
  cancel-in-progress: true

2. .gitattributes closes the CRLF trap for a fresh clone only.
Verified in review: git clone -c core.autocrlf=true at the merge head gives 0 of 71 .kt files CRLF. But a working tree that already existed before the branch keeps CRLF on every file the branch did not touch — git does not re-filter unchanged files on checkout — so 37 .kt files stay CRLF, spotlessCheck fails on MainActivity.kt, and ./gradlew won't even start (CRLF shebang → No such file or directory).

Self-service fix is git add --renormalize . or ./gradlew spotlessApply. This is one sentence next to the new CLAUDE.md command, not a code change — whoever hits it should not read it as a broken rule.

3. .gitattributes covers *.ttf/*.jar but no image or keystore types.
An Android repo acquires *.png/*.webp/*.jks sooner or later. Marking them -text now is cheaper than after a normalization pass has touched one.

4. Spotless is applied in :app only, targeting src/**/*.kt.
So the .gradle.kts build scripts sit outside the rule, and so does any module added later. Configuring it once at the root — for all projects, with a kotlinGradle target — makes the rule the repo's rather than :app's.

5. lintDebug reports a stale baseline.
"12 errors/warnings were listed in the baseline file but not found in the project" — 12 of the 18 entries in app/lint-baseline.xml are dead. Pre-existing, but the new PR gate now prints it on every run. Unrelated to #6's diff; cheap to clear while we are here.

Repo setting, not code

6. The check is advisory. The repo has zero branch-protection rules, so a red PR check is one click from being merged anyway. That still satisfies #3 — a reviewer sees a red X instead of hand-sorting imports — but "CI prevents it" needs a branch-protection rule on main requiring the check status. Deliberately not bundled into a code PR; someone should decide whether they want merges blocked before it gets turned on.

Known, no action

7. The PR gate tests the branch, not the merge result. The run log fetches +<sha>:refs/remotes/pull/6/head and checks that out. Green on a PR is not green after merge — the #7-on-#6 interaction had to be verified by hand. Worth knowing when a gate goes green on a stale branch.

8. The cache-warming claim did not hold. #6's description said a PR run warms the cache the main build reuses. Run #655 had identical keys and still downloaded platform-36_r02.zip, platform-tools and gradle-8.14.3-bin.zip from scratch, with restore-keys: gradle- in place — either Forgejo scopes caches per ref, or the 10-day-old entries had been evicted. The practical shape is that a PR check costs a cold ~4m30s. The description is merged and historical; nothing to change, just don't plan around the warm cache.

Items 1–5 are the actionable set.

#6 landed the spotless import-order rule and the PR gate that enforces it. The approving review left a set of non-blocking notes; this is where they live so they don't evaporate with the merge. Grouped by whether they are a code change, a repo setting, or a thing we only need to know. ## Code changes **1. `pr-checks.yml` has no `concurrency` and no `types:`.** Three pushes to one PR start three full cold runs, in parallel, on the single self-hosted runner. A cold run costs ~4m30s. Cheap version: ```yaml on: pull_request: types: [opened, synchronize, reopened] concurrency: group: pr-${{ github.ref }} cancel-in-progress: true ``` **2. `.gitattributes` closes the CRLF trap for a *fresh* clone only.** Verified in review: `git clone -c core.autocrlf=true` at the merge head gives 0 of 71 `.kt` files CRLF. But a working tree that already existed before the branch keeps CRLF on every file the branch did not touch — git does not re-filter unchanged files on checkout — so 37 `.kt` files stay CRLF, `spotlessCheck` fails on `MainActivity.kt`, and `./gradlew` won't even start (CRLF shebang → `No such file or directory`). Self-service fix is `git add --renormalize .` or `./gradlew spotlessApply`. This is one sentence next to the new `CLAUDE.md` command, not a code change — whoever hits it should not read it as a broken rule. **3. `.gitattributes` covers `*.ttf`/`*.jar` but no image or keystore types.** An Android repo acquires `*.png`/`*.webp`/`*.jks` sooner or later. Marking them `-text` now is cheaper than after a normalization pass has touched one. **4. Spotless is applied in `:app` only, targeting `src/**/*.kt`.** So the `.gradle.kts` build scripts sit outside the rule, and so does any module added later. Configuring it once at the root — for all projects, with a `kotlinGradle` target — makes the rule the repo's rather than `:app`'s. **5. `lintDebug` reports a stale baseline.** "12 errors/warnings were listed in the baseline file but not found in the project" — 12 of the 18 entries in `app/lint-baseline.xml` are dead. Pre-existing, but the new PR gate now prints it on every run. Unrelated to #6's diff; cheap to clear while we are here. ## Repo setting, not code **6. The check is advisory.** The repo has zero branch-protection rules, so a red PR check is one click from being merged anyway. That still satisfies #3 — a reviewer sees a red X instead of hand-sorting imports — but "CI prevents it" needs a branch-protection rule on `main` requiring the `check` status. Deliberately not bundled into a code PR; someone should decide whether they want merges blocked before it gets turned on. ## Known, no action **7. The PR gate tests the branch, not the merge result.** The run log fetches `+<sha>:refs/remotes/pull/6/head` and checks that out. Green on a PR is not green after merge — the #7-on-#6 interaction had to be verified by hand. Worth knowing when a gate goes green on a stale branch. **8. The cache-warming claim did not hold.** #6's description said a PR run warms the cache the main build reuses. Run #655 had identical keys and still downloaded `platform-36_r02.zip`, platform-tools and `gradle-8.14.3-bin.zip` from scratch, with `restore-keys: gradle-` in place — either Forgejo scopes caches per ref, or the 10-day-old entries had been evicted. The practical shape is that a PR check costs a cold ~4m30s. The description is merged and historical; nothing to change, just don't plan around the warm cache. Items 1–5 are the actionable set.
julian commented 2026-08-24 19:56:12 +02:00
Author
Owner
Copy link

Corrections from the #9 review, so this issue does not keep the wrong facts after the PR merges:

  • Item 1's premise is wrong. Forgejo already defaults cancel-in-progress to true for push and pull_request synchronize with no concurrency.group declared — this repo's run history shows #295 cancelled by #296 and #323 by #324, nobody did that by hand. "Three full cold runs at once" has never been possible here. types: [opened, synchronize, reopened] is also exactly the platform default. #9 keeps both blocks — explicit group, and cancel-in-progress extended to opened/reopened — but they save no runs today.
  • Item 2's self-service fix does not work. git add --renormalize . rewrites the index, never the working tree; every blob in HEAD is already LF, so in a stale clone it clears the M from git status and leaves the CRLF on disk. ./gradlew spotlessApply cannot run either — gradlew is one of the CRLF files. What rewrites the tree is git stash; git rm --cached -r . && git reset --hard; git stash pop (verified with git ls-files --eol). #9's CLAUDE.md paragraph says that now.
  • Item 5's count. app/lint-baseline.xml on main has 17 entries, not 18; 12 dead, 5 live. The no-op argument is unchanged.

Items 6–8 stand as written.

Corrections from the #9 review, so this issue does not keep the wrong facts after the PR merges: - **Item 1's premise is wrong.** Forgejo already defaults `cancel-in-progress` to true for `push` and `pull_request` `synchronize` with no `concurrency.group` declared — this repo's run history shows #295 cancelled by #296 and #323 by #324, nobody did that by hand. "Three full cold runs at once" has never been possible here. `types: [opened, synchronize, reopened]` is also exactly the platform default. #9 keeps both blocks — explicit group, and `cancel-in-progress` extended to `opened`/`reopened` — but they save no runs today. - **Item 2's self-service fix does not work.** `git add --renormalize .` rewrites the index, never the working tree; every blob in `HEAD` is already LF, so in a stale clone it clears the `M` from `git status` and leaves the CRLF on disk. `./gradlew spotlessApply` cannot run either — `gradlew` is one of the CRLF files. What rewrites the tree is `git stash; git rm --cached -r . && git reset --hard; git stash pop` (verified with `git ls-files --eol`). #9's `CLAUDE.md` paragraph says that now. - **Item 5's count.** `app/lint-baseline.xml` on main has 17 entries, not 18; 12 dead, 5 live. The no-op argument is unchanged. Items 6–8 stand as written.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
feat/schema-parity
No results found.
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
OpsDeck/mobile#8
No description provided.