unicode-org / jira-github-pr-check Goto Github PK

When the single-commit status fails, I've heard multiple times from multiple users that they don't realize that they can scroll down to the "jira-ticket" status to get to the details page that has the Squash Branch button. It might make it more discoverable if we add a link to the same page as "jira-ticket" from the "single-commit" status.

Make it easy to retrigger consolidate check if subchecks have passed

Example: unicode-org/icu#2161

add a basic travis that verifies that npm ci and the linter are working

At least, disallow PRs to have no fixVersion set.
A bonus would be to disallow PRs without the 'correct' fixVersion (but this is more challenging to calculate)

see CLDR-15400

Solution we could have one or more custom fields, updated by the pr checker / commit checker, which gives us the correct state of the ticket re: the repository.

When merging from a maint branch to master, the tool should remind the developer not to use the rebase-merge button. There does not appear to be a way for the tool to prevent the use of the rebase merge button, but at least it could leave a status that reminds the developer and makes the button non-green.

unicode-org/icu#539

I don't know what 'creating a status is', but because there's a github credential, it might be good to use a secret (there's a field for it) in connecting to the webhook.

https://github.com/unicode-org/jira-github-pr-check/blob/master/app.js#L308-L322

POST /touch/unicode-org/cldr/939 422

nothing in the logs as to why it's not completing.

reported by @Kristi27

clicking 'update status' gets a spinning spinner, but the console says:

[Error] Failed to load resource: the server responded with a status of 422 () (1364, line 0). on https://us-central1-dev-infra-273822.cloudfunctions.net/unicode-github-bot/touch/unicode-org/cldr/1364

FE should detect this and show a failure.

It would be handy if the PR checker added a convenience link to, for example, https://cla-assistant.io/check/unicode-org/cldr?pullRequest=1878 to enable rechecking of PRs

the detail page such as https://us-central1-festive-cistern-138116.cloudfunctions.net/icu-jira-github-pr-check-test/info/unicode-org/icu/49 should link to:

• this repository ( I like to do require('./package.json').repository.url ) 'fork me on github'- but also so folks can find the source
• a static (configurable) link to something like http://site.icu-project.org/processes/contribute "for more detail on our process" [page out of date of course, but still]

related to #32 and #24

• somehow error if a PR on main against a future fixversion (i.e. v42 when we're actually working on v41)
• or, error if a PR on maint branch not against that branch's version

Lots of challenges with this, especially how to keep the list up to date.

Jira is discontinuing passwords for auth to REST APIs on a very short timeframe.

https://confluence.atlassian.com/cloud/api-tokens-938839638.html

@srl295

related to #24

When touching a PR with a Jira ticket with a long title, the checker attempts to set a status that is over 140 characters.

see also #30

jira-ticket — Commit message for 9d1c3e3 fails validation

… is not very helpful as to what should be present, especially for a message that does contain CLDR-15180 somewhere in it.

Proposal:

• If a valid JIRA id (even on a loose match - match Cldr:15180 or similar) then suggest as follows:

Bad Commit message for 9d1c3e3:  'CLDR-15180 Describe the change here'

Describe the change here could be a static string, or could be calculated starting with the first alpha character.

That way if we get a message such as Cldr-15180 - /bin/sh was not found then it will instead say: CLDR-15180 bin/sh was not found (Extraneous: ' - /') - and show what part was causing the validation failure.

It seems the tool does not allow branches to be squashed if I am not the author but a reviewer. On the command line, I can push to PRs where I am a reviewer.

CLDR has a new Umbrella type. Tickets against that type should not allow commits or PRs.

Context: https://github.community/t5/How-to-use-Git-and-GitHub/Auto-approve-force-pushes-if-PR-was-already-approved/m-p/15029/thread-id/4672

The checker tool should be able to detect when the only change to a PR was a force-push to squash, edit commit message, etc., and it should automatically re-approve the PR.

If not auto-approve, the bot should at least leave a comment saying that the code is the same, so that the human reviewer doesn't have to take that on faith.

@srl295

@sffc this is somewhat of a question.

• Does the pr-check get notified when a PR is merged?

Context: https://unicode-org.atlassian.net/browse/CLDR-15400

There's sometimes confusion when a PR or commit mentions a JIRA ticket but isn't targetted at that ticket. xrefs are super helpful, but can add noise. (I filed this ticket to try to avoid needing some kind of policy that restricts mentioning other tickets.)

Jira's "developer" field isn't really a field, you can't really search for "Tickets that have commits" and even if you could, you don't know if the ticket was really fixed by the PR or just mentioned by it.

If pr-check gets notified on merge, then it could actually update a Jira field, something like "mergedIn".

• For branch maint/maint-43 it could definitely add maint-43 to mergedIn.
• For branch main it could add main to mergedIn

More advanced:

• We could somehow use the current version logic to actually use the "targeted release version" for that branch for the mergedIn field.
• mergedIn should be a list, not a singleton, because of cherry picks.

The tool should have a button from the PR error page that fixes up the commit messages for you and overwrites the branch with a squash commit, where the commit message is the PR title. The tool can ask the user for OAuth write permission to their repository and perform the fix in one or two clicks.

@srl295

See #37 referring to #36

If the PR title is something like Cldr-1234 - Something Here could the checker actually modify the PR title and change it to CLDR-1234 Something Here automatically?

a3de9a3 supports DISABLE_JIRA_ISSUE_MATCH=true

Instead, how about something like this:

DisableJiraIssueMatch: because this is a merge from the maint branch

In other words, a special token at the beginning, and then a required description.

Can text follow DISABLE_JIRA_ISSUE_MATCH=true? Then:

DISABLE_JIRA_ISSUE_MATCH=true: because this is a merge from the maint branch

would work. but a human readable description is important.

When a PR is in early development, the bot can be noisy. Given that the bot's primary purpose is to help with code reviews, there should be an option (or perhaps make it the default behavior) to suppress notifications if a PR is in Draft state.

CC @Manishearth

Currently if two authors push to a branch, it looks like jira-github-pr-check squashes to a commit attributed to the last author. A small fix by a reviewer can thus result in the reviewer getting full blame - thus discouraging direct fixes.

Options:

• refusing to squash when there's more than one author?
• using the first author, not the last?
• or at least warning very clearly when there's more than one?
• some way of explicitly choosing which author?
• (only squashing successive commits by the same author?) <- not recommended, when the same author might also commit with different email addresses. And the end-result isn't a single commit.

I like the first option above: I think it's reasonable to just let people squash manually when there's more than one author.

• document how to test locally
• document how to test with webhooks

PRs such as unicode-org/icu#1663 can get merged to master/main accidentally, even if they should have landed on the maint branch. Consider using the Jira bot to leave a warning status to make this harder to do unintentionally.

Could the hook indicate whether all commits are eligible for a "rebase and merge"? i.e. if they all mention the same JIRA id AND include the PR URL?

Right now, the check is wired up for only public tickets; there is no authentication to Jira. This means that tickets not viewable publicly, such as those with "sensitive" security level. It's unclear whether this is a bug or a feature.

@markusicu @srl295

The checker tool's page ( https://us-central1-festive-cistern-138116.cloudfunctions.net/icu-jira-github-pr-check-test/info/unicode-org/cldr/13 ) currently says:

Commit a390c8f is for CLDR-1234, but the PR is for CLDR-11910; to disable, set DISABLE_JIRA_ISSUE_MATCH=true in the PR description

This page should recommend fixing the bad commit message as the first resort rather than bypassing the check.

// @macchiati

If the state of a ticket changes after the PR is merged, the check status should continue to reflect the state at the time of merging. To fix this, the checker should not be able to modify PRs that have been merged or closed.

Should the bot also require a PR owner?

Upside is that ownership shows up earlier in lists of PRs.

Downside is that community contributions, that might otherwise pass tests, will show with an X as broken when they might be otherwise OK.

# Option 1: Personal Access Token; easiest and useful for testing.
# Create one of these from https://github.com/settings/tokens
GITHUB_TOKEN=xxxxxxxxxx

I'm guessing repo:status scope is what's needed?

CLDR-TC consensus:

• lock PRs after merge
• add a comment, something like: This is merged, please comment on CLDR-ABCD if you have further comments

https://unicode-org.atlassian.net/browse/CLDR-16143

see #36 - could the 'squash' function figure out what needs to be fixed on the commit message and fix it?

over on unicode-org/cldr#1085 the following text failed both PR title and Commit title validation:

• CLDR-14395 /api/auth endpoints

with "PR must begin with a Jira ticket ID" etc.

Changing to CLDR-14395 API for … fixed it. Something with slashes?

I believe that this is now the last of the unicode-org repos which is still using "master". (Except for archived repos.)
According to the notes on our tracking spreadsheet, this was tried before and "changed back to master due to GCP bug".
Could someone please look at what bug that was and whether we can get past it now?

@srl295 @sffc

add a simple docker file to simplify deployment