Skip to content

Commit 717095a

Browse files
authored
Fix canary build links with a rebuilt UI (#860)
On a release build, pressing install on a canary opened the newest release. The channel rule that keeps nightlies out of the update card also filtered the list an explicit request was resolved against, so the version code the canary list passed matched nothing and the selection fell back to the channel default. That rule now filters what is offered, not what can be asked for by name. The canary page is rebuilt around the builds themselves. Each row is the build's head commit — subject wrapped, author credited, pull request in a fixed corner that opens the discussion — matched by SHA, falling back to the subject CI writes into the release notes. The header names the issues closed since the running build was cut, read from the issues endpoint filtered to `completed`: the Development panel closes issues without writing to any commit message, and the link itself exists only in GraphQL, which needs an account. The commit rail's marker shows where the running build sits, and a build wearing a canary's number without being it is told so.
1 parent abae837 commit 717095a

28 files changed

Lines changed: 1254 additions & 258 deletions

File tree

manager/src/main/kotlin/org/matrix/vector/manager/data/github/GitHubModels.kt

Lines changed: 54 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -247,30 +247,60 @@ data class GhReleaseAsset(
247247
@SerialName("browser_download_url") val downloadUrl: String? = null,
248248
)
249249

250-
/** A successful CI run, as the canary screen renders it. */
251-
data class CanaryBuild(
252-
val id: Long,
253-
/**
254-
* The build this is, and the key the installer selects by.
255-
*
256-
* CI tags every canary `canary-<versionCode>`, so the number is in the tag and needs no second
257-
* request. It is what [FrameworkRelease.versionCode] holds for the same release, which is how a
258-
* row here can hand the installer a build rather than a URL.
259-
*/
260-
val versionCode: Long,
250+
/**
251+
* A closed issue, as GitHub's issue list reports it.
252+
*
253+
* **This is how the canary screen knows what got fixed, and it has to be.** A commit message only
254+
* names an issue when somebody wrote `Fixes #816` into it; this repository's issues are usually
255+
* linked through the web UI's *Development* panel instead, which closes them on merge and writes
256+
* nothing into the history at all. The link itself is only readable through GraphQL —
257+
* `PullRequest.closingIssuesReferences` — and GraphQL answers 403 to an anonymous caller, which
258+
* this app is by design. So what is asked instead is the question REST will answer without an
259+
* account: which issues closed, and when.
260+
*/
261+
data class ClosedIssue(
262+
val number: Int,
261263
val title: String,
262-
val branch: String,
263-
val shortSha: String,
264-
val epochSeconds: Long,
264+
val closedAtEpoch: Long,
265265
val htmlUrl: String?,
266-
val artifacts: List<CanaryArtifact>,
267266
)
268267

268+
@Serializable
269+
data class GhIssue(
270+
val number: Int,
271+
val title: String = "",
272+
@SerialName("closed_at") val closedAt: String? = null,
273+
/**
274+
* Why it closed: `completed`, `not_planned` or `duplicate`.
275+
*
276+
* Only the first is a fix. Counting the others would tell a reader that eleven issues were
277+
* dealt with since their build when five of them were triage.
278+
*/
279+
@SerialName("state_reason") val stateReason: String? = null,
280+
@SerialName("html_url") val htmlUrl: String? = null,
281+
) {
282+
/**
283+
* Whether this is really a pull request.
284+
*
285+
* The issues endpoint returns both — a pull request *is* an issue to GitHub — and in one page
286+
* of this repository's closed items more than half were pull requests. Told apart by the URL
287+
* rather than by the presence of the `pull_request` object, which would mean decoding a nested
288+
* payload none of whose fields are wanted.
289+
*/
290+
val isPullRequest: Boolean
291+
get() = htmlUrl?.contains("/pull/") == true
292+
}
293+
269294
/**
270295
* A published build of the framework, canary or stable, with the zip to flash.
271296
*
272297
* One type for both channels because the install path is identical — the difference is only which
273298
* of them a given reader is allowed to be offered.
299+
*
300+
* One type for the canary list as well, which used to read the same endpoint through a shape of its
301+
* own. Two models over one response meant the canary page fetched what the update page was already
302+
* holding, and could say nothing about a build that this type does not carry — no notes, no commit,
303+
* and so no way to mark the one that is running.
274304
*/
275305
data class FrameworkRelease(
276306
val tag: String,
@@ -302,6 +332,15 @@ data class FrameworkRelease(
302332
/** The one to offer by default when nothing has been chosen. */
303333
val defaultZip: CanaryArtifact?
304334
get() = zips.firstOrNull { it.variant == ZipVariant.Release } ?: zips.firstOrNull()
335+
336+
/**
337+
* The commit, abbreviated the way git abbreviates it, or null when the release names a branch.
338+
*
339+
* Seven characters because that is what `git rev-parse --short` gives on a repository this
340+
* size, and what the commit rail already prints — the two are read side by side.
341+
*/
342+
val shortSha: String?
343+
get() = commit?.take(7)
305344
}
306345

307346
/**
@@ -321,7 +360,6 @@ data class CanaryArtifact(
321360
val id: Long,
322361
val name: String,
323362
val sizeInBytes: Long,
324-
val expired: Boolean,
325363
val downloadUrl: String?,
326364
) {
327365
val variant: ZipVariant

manager/src/main/kotlin/org/matrix/vector/manager/data/github/GitHubRepository.kt

Lines changed: 59 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -705,57 +705,66 @@ class GitHubRepository(
705705
.getOrNull()
706706

707707
/**
708-
* The canary builds, newest first.
709-
*
710-
* These are **prereleases**, not Actions artifacts, and that is the whole point. GitHub gates an
711-
* artifact download behind an account even for a public repository — `actions/artifacts/<id>/zip`
712-
* answers 401 to an anonymous caller, while a release asset answers 206 — so sourcing canaries
713-
* from artifacts would mean asking every would-be tester for an OAuth grant to work around a
714-
* storage decision. CI attaches the same zips to a rolling `canary-<versionCode>` prerelease,
715-
* and this reads that, so nobody signs in to anything.
716-
*
717-
* Filtered to the canary tag rather than taking every prerelease: a hand-cut release candidate
718-
* is also a prerelease, and it is not a nightly.
708+
* The issues that have been closed as done, newest first.
709+
*
710+
* **Asked of the issue tracker rather than derived from the commits, and that is not a
711+
* shortcut.** An issue linked through GitHub's *Development* panel — the usual way here — is
712+
* closed by the merge itself and leaves no trace in any commit message, so reading the history
713+
* would report only the minority that happened to be written up as `Fixes #816`. The link that
714+
* did the closing lives in `PullRequest.closingIssuesReferences`, which exists only in GraphQL,
715+
* and GraphQL answers 403 without an account. This endpoint answers the neighbouring question
716+
* anonymously, and the answer is the one worth showing: not which commit closed what, but what
717+
* has been fixed since the reader's build.
718+
*
719+
* Closed is not fixed: `not_planned` and `duplicate` are also closures, and were a fifth of one
720+
* page here. Only `completed` is counted.
721+
*
722+
* One page, unpaginated. A hundred items reaches back several weeks on this repository, which
723+
* covers the span between any two builds a reader could be choosing between; older than that
724+
* and the number stops mattering because the reader is being told to update, not to test.
719725
*/
720-
suspend fun canaryBuilds(freshness: Freshness = Freshness.Revalidate): List<CanaryBuild> =
726+
suspend fun closedIssues(freshness: Freshness = Freshness.Revalidate): List<ClosedIssue> =
721727
withContext(Dispatchers.IO) {
722-
val body = releaseListJson(freshness) ?: return@withContext emptyList()
723-
724-
runCatching { json.decodeFromString<List<GhRelease>>(body) }
725-
.onFailure { e -> logE("update: canary release list unreadable", e) }
728+
val url = "$API/$REPO/issues?state=closed&per_page=100&sort=updated&direction=desc"
729+
val body =
730+
runCatching { get(url, freshness) }
731+
.onFailure { e -> logW("canary: closed issue list unavailable", e) }
732+
.getOrNull() ?: return@withContext emptyList()
733+
734+
runCatching { json.decodeFromString<List<GhIssue>>(body) }
735+
.onFailure { e -> logE("canary: closed issue list unreadable", e) }
726736
.getOrDefault(emptyList())
727-
.filter { it.prerelease && it.tagName.startsWith(CANARY_TAG_PREFIX) }
728-
.take(CANARY_KEEP)
729-
.map { release ->
730-
CanaryBuild(
731-
id = release.id,
732-
versionCode = release.versionCode() ?: 0,
733-
title = release.name ?: release.tagName,
734-
branch = release.tagName,
735-
shortSha = release.targetCommitish.take(7),
736-
epochSeconds = parseIso8601(release.publishedAt.orEmpty()),
737-
htmlUrl = release.htmlUrl,
738-
artifacts =
739-
release.assets.map {
740-
CanaryArtifact(
741-
id = it.id,
742-
name = it.name,
743-
sizeInBytes = it.size,
744-
expired = false,
745-
downloadUrl = it.downloadUrl,
746-
)
747-
},
737+
.filter { !it.isPullRequest && it.stateReason == "completed" }
738+
.mapNotNull { issue ->
739+
val closed = parseIso8601(issue.closedAt ?: return@mapNotNull null)
740+
ClosedIssue(
741+
number = issue.number,
742+
title = issue.title,
743+
closedAtEpoch = closed.takeIf { it > 0 } ?: return@mapNotNull null,
744+
htmlUrl = issue.htmlUrl,
748745
)
749746
}
747+
.sortedByDescending { it.closedAtEpoch }
750748
}
751749

752750
/**
753751
* Every published build, both channels, newest first.
754752
*
755-
* One fetch for both because they come from the same endpoint, and because deciding which
756-
* channel a reader is on needs to see both: a canary that has aged out of the rolling five is
757-
* still recognisable as a canary by being *newer than the newest stable release*, and that
758-
* comparison is impossible with only one of the two lists in hand.
753+
* The canaries here are **prereleases**, not Actions artifacts, and that is the whole point.
754+
* GitHub gates an artifact download behind an account even for a public repository —
755+
* `actions/artifacts/<id>/zip` answers 401 to an anonymous caller, while a release asset answers
756+
* 206 — so sourcing canaries from artifacts would mean asking every would-be tester for an OAuth
757+
* grant to work around a storage decision. CI attaches the same zips to a rolling
758+
* `canary-<versionCode>` prerelease, and this reads those, so nobody signs in to anything.
759+
*
760+
* A canary is recognised by its tag rather than by being a prerelease: a hand-cut release
761+
* candidate is also a prerelease, and it is not a nightly.
762+
*
763+
* One fetch for both channels because they come from the same endpoint, because deciding which
764+
* channel a reader is on needs to see both — a canary that has aged out of the rolling five is
765+
* still recognisable by being *newer than the newest stable release*, and that comparison is
766+
* impossible with only one of the two lists in hand — and because the canary list is this same
767+
* answer filtered, not a second question.
759768
*/
760769
suspend fun frameworkReleases(freshness: Freshness = Freshness.Revalidate):
761770
List<FrameworkRelease> =
@@ -788,7 +797,6 @@ class GitHubRepository(
788797
id = it.id,
789798
name = it.name,
790799
sizeInBytes = it.size,
791-
expired = false,
792800
downloadUrl = it.downloadUrl,
793801
)
794802
},
@@ -945,7 +953,14 @@ class GitHubRepository(
945953

946954
/** CI keeps five; a few extra are fetched so a stable release among them costs nothing. */
947955
private const val CANARY_FETCH = 12
948-
private const val CANARY_KEEP = 5
956+
957+
/**
958+
* How many canaries CI keeps, which the canary screen states as reassurance.
959+
*
960+
* Read from here rather than written into the sentence, so the promise the screen makes
961+
* and the number the workflow prunes to cannot drift apart silently.
962+
*/
963+
const val CANARY_KEEP = 5
949964

950965
private const val API = "https://api.github.com/repos"
951966
private const val API_ROOT = "https://api.github.com"
@@ -982,6 +997,7 @@ class GitHubRepository(
982997

983998
private val PR_SUFFIX = Regex("""\(#(\d+)\)\s*$""")
984999

1000+
9851001
private val LAST_PAGE = Regex("""[?&]page=(\d+)>;\s*rel="last"""")
9861002

9871003
private val CO_AUTHOR =

0 commit comments

Comments
 (0)