WIP: Address various issues about milestones by rimrul · Pull Request #143 · git-for-windows/gfw-helper-github-app

rimrul · 2025-06-12T09:33:45Z

I still need to add some tests for this.

This was originally supposed to be just a PR to address #5, but I've had a few realizations while working on this:

It makes a lot of sense to think about all three milestone issues (Automatically add merged component-update tickets to the latest milestone #5, Automatically rename the latest milestone when a PR is opened to rebase to the latest Git version #6, Automatically close the latest milestone upon release #7) while working on one of them
WIP: Automatically close Git for Windows' milestones after a release #67 lays some good ground work
We need to talk about the details of the intended workflow.
Writing the tests for this only really makes sense after we've talked about the workflow.
The best trigger for Automatically close the latest milestone upon release #7 might be the released action of the release webhook

The workflow that #5, #6 and #7 outline is as follows:

component-update issues get added to the current Next release milestone when the corresponding PR gets merged.
the current Next release milestone gets renamed when a new Git version PR is created. A New Next release milestone is created
When the PR is merged and the release published, the renamed milestone is closed

#67 outlines a different order of events

The current Next release milestone is closed and an new Next release milestone is created (at some point)
The old milestone is then manually renamed? (at some point)

What we currently have is also different.

The old Next release milestone has been renamed and a new Next release milestone has been created before the PR has been created.

None of this is gonna be an issue, but the timing deviation in the current rename and recreate raises the question why we rename and recreate. And the answer is that we're sometimes close to a current release and already want to plan something (in this case git-for-windows/git#5671) for after this release.

But we also have reasons to wait with the name and recreate action until shortly before a release. One is that out-of-band releases also usually get their own milestone, but we don't usually know early on that a Next release milestone will be for an out-of-band-release. Having a fixed name for the current milestone also makes automation (#5) easier.

This leads to the discussion how we want to handle these edge cases in the future. Do we want to wait for the automation to rename and recreate in cases like git-for-windows/git#5671 or does the automation need to be aware that we might have manually renamed? And do we care about Situations like git-for-windows/git#5411 (comment) where a late component update would already be assigned to the new milestone?

How should the automation decide when it's time to close the milestone? I think the released action of the release webhook fits our needs pretty exactly.

When should the automation do the renaming and recreating for an out-of-band release? Do we just manually do it in that case? Or should the closing step detect that it hasn't happened yet and do it then?

Co-authored-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Matthias Aßhauer <mha1993@live.de>

This fixes git-for-windows#5 Signed-off-by: Matthias Aßhauer <mha1993@live.de>

… latest Git version This fixes git-for-windows#6 Co-authored-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Matthias Aßhauer <mha1993@live.de>

This fixes git-for-windows#7 Co-authored-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Matthias Aßhauer <mha1993@live.de>

dscho · 2025-06-13T07:52:08Z

You raise a lot of good points. I guess that my renaming of "Next release" to "v2.50.0" was premature, and I should have instead created an "After next release".

In general, I am a big fan of automating everything, not only so that I cannot forget crucial steps by mistake, but also to implicitly document Git for Windows' workflow.

As such, I think that we should settle on what you described:

The workflow that #5, #6 and #7 outline is as follows:

component-update issues get added to the current Next release milestone when the corresponding PR gets merged.

the current Next release milestone gets renamed when a new Git version PR is created. A New Next release milestone is created

When the PR is merged and the release published, the renamed milestone is closed

dscho

Wow, great work!

I added a couple of suggestions, hopefully you agree with them?

dscho · 2025-06-13T07:54:23Z

GitForWindowsHelper/milestones.js

+const getMilestoneByName = async (context, token, owner, repo, name) => {
+    const githubApiRequest = require('./github-api-request')
+    const milestones = await githubApiRequest(context, token, 'GET', `/repos/${owner}/${repo}/milestones?state=open`)
+    if (milestones.length === 2) {


We should probably be more lenient and test > 1 here (and then use .splice(0, milestones.length, filtered) below).

I've meant to ask about that.

dscho · 2025-06-13T07:57:29Z

GitForWindowsHelper/update-milestones.js

+    const getToken = (() => {
+        let token
+
+        const get = async () => {
+            const getInstallationIdForRepo = require('./get-installation-id-for-repo')
+            const installationId = await getInstallationIdForRepo(context, owner, repo)
+            const getInstallationAccessToken = require('./get-installation-access-token')
+            return await getInstallationAccessToken(context, installationId)
+        }
+
+        return async () => token || (token = await get())
+    })()
+
+    const isAllowed = async (login) => {
+        if (login === 'gitforwindowshelper[bot]') return true
+        const getCollaboratorPermissions = require('./get-collaborator-permissions')
+        const token = await getToken()
+        const permission = await getCollaboratorPermissions(context, token, owner, repo, login)
+        return ['ADMIN', 'MAINTAIN', 'WRITE'].includes(permission.toString())
+    }
+
+    if (!await isAllowed(sender)) throw new Error(`${sender} is not allowed to do that`)


I guess it is about high time to move this (duplicated) logic into a central place instead, and maybe now would be as good a time as any to do that?

That thought has crossed my mind as well.

dscho · 2025-06-13T08:13:41Z

GitForWindowsHelper/update-milestones.js

+    const candidates = req.body.pull_request.body.match(/(?:[Cc]loses|[Ff]ixes) (?:https:\/\/github\.com\/git-for-windows\/git\/issues\/|#)(\d+)/)
+
+    if (candidates.length !== 1) throw new Error(`Expected 1 candidate issue, got ${candidates.length}`)


I was looking for something more robust, and while there does not seem anything useful in GitHub's REST API, there is something in the GraphQL API. Try this, for example:

gh api graphql -f query='{ repository(owner: "git-for-windows", name: "MSYS2-packages") { pullRequest(number: 236) { closingIssuesReferences(first: 10) { nodes { number repository { owner { login } name } title url } } } } }'

Over here, this returns this highly useful answer:

{ "data": { "repository": { "pullRequest": { "closingIssuesReferences": { "nodes": [ { "number": 5656, "repository": { "owner": { "login": "git-for-windows" }, "name": "git" }, "title": "[New curl version] 8.14.1", "url": "https://github.com/git-for-windows/git/issues/5656" } ] } } } } }

We already have a precedent of a GraphQL call when looking for the "collaborator permission", so we could something similar here.

That does look immensely more helpful than the REST API

dscho · 2025-06-13T08:16:33Z

GitForWindowsHelper/index.js

+    try {
+        const {renameCurrentAndCreateNextMilestone} = require('./update-milestones')
+        if (req.headers['x-github-event'] === 'pull_request'
+            && req.body.repository.full_name === 'git-for-windows/git'
+            && req.body.action === 'opened'
+            && req.body.pull_request.merged === 'true') return ok(await renameCurrentAndCreateNextMilestone(context, req))
+    } catch (e) {
+        context.log(e)
+        return withStatus(500, undefined, e.message || JSON.stringify(e, null, 2))
+    }
+


I'd rather do that when publishing the final release, i.e. at the same time as the current milestone is closed.

That would prevent issues with late component updates.

Isn't it the other way round? When a final rebase PR is opened, and /git-artifacts produces an installer that has issues that require a component-update ticket to be addressed, we'd still want that in the same milestone, right?

dscho · 2025-06-13T08:20:29Z

GitForWindowsHelper/index.js

+        if (req.headers['x-github-event'] === 'pull_request'
+            && req.body.repository.full_name === 'git-for-windows/git'
+            && req.body.action === 'closed'
+            && req.body.pull_request.merged === 'true') return ok(await closeReleaseMilestone(context, req))


Alternatively, we could piggy-back onto the code that "pushes" the PR branch to main. That would cut out one extra round trip between GitHub and the GitForWindowsHelper, and simplify the mental model of the process.

It does cut out a roundtrip, but it does require duplication of the "is this a pre-release" logic from https://github.com/git-for-windows/git-for-windows-automation/blob/main/.github/actions/github-release/action.yml and maybe some special handling of out of band releases, whereas the release.released approach would get that for free.

Ah, I missed that. But wouldn't we want much more narrow conditions to be met before calling closeReleaseMilestone()?

rimrul · 2025-06-13T09:23:57Z

I guess that my renaming of "Next release" to "v2.50.0" was premature, and I should have instead created an "After next release".

I've just had the thought that we could always have an "After next release" milestone and have that automatically rotated into becoming the new "Next release" milestone and a new "After next release" created.

We don't ususally need a milestone for the broad concept of "at some point in the future", but "the release after the next" is occasionally useful.

dscho · 2025-06-13T10:52:59Z

I've just had the thought that we could always have an "After next release" milestone and have that automatically rotated into becoming the new "Next release" milestone and a new "After next release" created.

I like it!

rimrul linked an issue Jun 12, 2025 that may be closed by this pull request

Automatically add merged component-update tickets to the latest milestone #5

Open

rimrul force-pushed the add-issues-to-milestone branch from b5c2225 to 5936463 Compare June 12, 2025 09:35

add a helper script for working with milestones

a27115e

Co-authored-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Matthias Aßhauer <mha1993@live.de>

rimrul force-pushed the add-issues-to-milestone branch 2 times, most recently from 8548d6e to 3201a44 Compare June 12, 2025 12:36

rimrul and others added 3 commits June 12, 2025 14:38

WIP: Add component update issues to "Next release" milestone

adc0fb5

This fixes git-for-windows#5 Signed-off-by: Matthias Aßhauer <mha1993@live.de>

WIP: rename the latest milestone when a PR is opened to rebase to the…

b6e6295

… latest Git version This fixes git-for-windows#6 Co-authored-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Matthias Aßhauer <mha1993@live.de>

WIP: After publishing a release, close the corresponding milestone

3e20881

This fixes git-for-windows#7 Co-authored-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Matthias Aßhauer <mha1993@live.de>

rimrul force-pushed the add-issues-to-milestone branch from 3201a44 to 3e20881 Compare June 12, 2025 12:38

rimrul changed the title ~~WIP: Add component update issues to "Next release" milestone~~ WIP: Address various issues about milestones Jun 12, 2025

rimrul requested a review from dscho June 12, 2025 13:36

dscho reviewed Jun 13, 2025

View reviewed changes

		const candidates = req.body.pull_request.body.match(/(?:[Cc]loses\|[Ff]ixes) (?:https:\/\/github\.com\/git-for-windows\/git\/issues\/\|#)(\d+)/)

		if (candidates.length !== 1) throw new Error(`Expected 1 candidate issue, got ${candidates.length}`)

Conversation

rimrul commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dscho commented Jun 13, 2025

Uh oh!

dscho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rimrul commented Jun 13, 2025

Uh oh!

dscho commented Jun 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rimrul commented Jun 12, 2025 •

edited

Loading