Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .github/workflows/dev_pr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ concurrency:
permissions:
contents: read
pull-requests: write
issues: write

jobs:
process:
Expand Down Expand Up @@ -66,7 +67,7 @@ jobs:
const script = require(`${process.env.GITHUB_WORKSPACE}/.github/workflows/dev_pr/title_check.js`);
script({github, context});

- name: Check Jira Issue
- name: Check Issue
if: |
(github.event.action == 'opened' ||
github.event.action == 'edited')
Expand All @@ -75,7 +76,7 @@ jobs:
debug: true
github-token: ${{ secrets.GITHUB_TOKEN }}
script: |
const script = require(`${process.env.GITHUB_WORKSPACE}/.github/workflows/dev_pr/jira_check.js`);
const script = require(`${process.env.GITHUB_WORKSPACE}/.github/workflows/dev_pr/issue_check.js`);
script({github, context});

- name: Assign GitHub labels
Expand Down
64 changes: 41 additions & 23 deletions .github/workflows/dev_pr/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,34 +18,33 @@
const https = require('https');

/**
* Given the title of a PullRequest return the ID of the JIRA issue
* Given the title of a PullRequest return the Issue
*
* @param {String} title
* @returns {String} the ID of the associated JIRA issue
* @returns {Issue} or null if no issue detected.
*
* @typedef {Object} Issue
* @property {string} kind - The kind of issue: minor, jira or github
* @property {string} id - The id of the issue:
* ARROW-XXXX, PARQUET-XXXX for jira
* The numeric issue id for github
*/
function detectJIRAID(title) {
function detectIssue(title) {
if (!title) {
return null;
}
const matched = /^(WIP:?\s*)?((ARROW|PARQUET)-\d+)/.exec(title);
if (!matched) {
return null;
if (title.startsWith("MINOR: ")) {
return {"kind": "minor"};
}
return matched[2];
}

/**
* Given the title of a PullRequest checks if it contains a JIRA issue ID
* @param {String} title
* @returns {Boolean} true if it starts with a JIRA ID or MINOR:
*/
function haveJIRAID(title) {
if (!title) {
return false;
const matched_jira = /^(WIP:?\s*)?((ARROW|PARQUET)-\d+)/.exec(title);
if (matched_jira) {
return {"kind": "jira", "id": matched_jira[2]};
}
if (title.startsWith("MINOR: ")) {
return true;
const matched_gh = /^(WIP:?\s*)?(GH-)(\d+)/.exec(title);
if (matched_gh) {
return {"kind": "github", "id": matched_gh[3]};
}
return /^(WIP:?\s*)?(ARROW|PARQUET)-\d+/.test(title);
return null;
}

/**
Expand All @@ -69,8 +68,27 @@ async function getJiraInfo(jiraID) {
});
}

/**
* Retrieves information about a GitHub issue.
* @param {String} issueID
* @returns {Object} the information about a GitHub issue.
*/
async function getGitHubInfo(github, context, issueID, pullRequestNumber) {
try {
const response = await github.issues.get({
issue_number: issueID,
owner: context.repo.owner,
repo: context.repo.repo,
})
return response.data
} catch (error) {
console.log(`${error.name}: ${error.code}`);
return false
}
}

module.exports = {
detectJIRAID,
haveJIRAID,
getJiraInfo
detectIssue,
getJiraInfo,
getGitHubInfo
};
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,16 @@

const helpers = require("./helpers.js");

/**
* Performs checks on the JIRA Issue:
* - The issue is started in JIRA.
* - The issue contains components.
*
* @param {Object} github
* @param {Object} context
* @param {String} pullRequestNumber
* @param {String} jiraID
*/
async function verifyJIRAIssue(github, context, pullRequestNumber, jiraID) {
const ticketInfo = await helpers.getJiraInfo(jiraID);
if(!ticketInfo["fields"]["components"].length) {
Expand All @@ -30,6 +40,13 @@ async function verifyJIRAIssue(github, context, pullRequestNumber, jiraID) {
}
}

/**
* Adds a comment to add components on the JIRA ticket.
*
* @param {Object} github
* @param {Object} context
* @param {String} pullRequestNumber
*/
async function commentMissingComponents(github, context, pullRequestNumber) {
const {data: comments} = await github.issues.listComments({
owner: context.repo.owner,
Expand All @@ -54,6 +71,13 @@ async function commentMissingComponents(github, context, pullRequestNumber) {
}
}

/**
* Adds a comment to start the ticket in JIRA.
*
* @param {Object} github
* @param {Object} context
* @param {String} pullRequestNumber
*/
async function commentNotStartedTicket(github, context, pullRequestNumber) {
const {data: comments} = await github.issues.listComments({
owner: context.repo.owner,
Expand All @@ -78,11 +102,73 @@ async function commentNotStartedTicket(github, context, pullRequestNumber) {
}
}

/**
* Assigns the Github Issue to the PR creator.
*
* @param {Object} github
* @param {Object} context
* @param {String} pullRequestNumber
* @param {Object} issueInfo
*/
async function assignGitHubIssue(github, context, pullRequestNumber, issueInfo) {
await github.issues.addAssignees({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: issueInfo.number,
assignees: context.payload.pull_request.user.login
});
await github.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: pullRequestNumber,
body: ":warning: GitHub issue #" + issueInfo.number + " **has been automatically assigned in GitHub** to PR creator."
});
}

/**
* Performs checks on the GitHub Issue:
* - The issue is assigned to someone. If not assign it gets automatically
* assigned to the PR creator.
* - The issue contains any label.
*
* @param {Object} github
* @param {Object} context
* @param {String} pullRequestNumber
* @param {String} issueID
*/
async function verifyGitHubIssue(github, context, pullRequestNumber, issueID) {
const issueInfo = await helpers.getGitHubInfo(github, context, issueID, pullRequestNumber);
if (issueInfo) {
if (!issueInfo.assignees.length) {
await assignGitHubIssue(github, context, pullRequestNumber, issueInfo);
}
if(!issueInfo.labels.length) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably check if there is a label["name"] that starts with Component:, I don't think that we want to accept any label.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment several component labels are not prefixed with Component:, i.e. all the language related ones. @jorisvandenbossche I think you added some of the missing labels, are we planning to change the language ones? I was planning to add more validation around Components labels in the future but I am happy to improve it now if we are planning to update those.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would maybe handle this later / separately? I didn't yet rename existing labels in the assumption that those are used by existing workflows as well that would have to be updated (i.e. labeler.yml). Although that would probably an easy PR to make to rename the labels in labeler.yml

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI in #14750 I have been taking for granted that component labels are those prefixed with "Component: " to identify the component of an issue, so would be helpful to be consistent

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we implement duplicate labels now for those component that already have non-prefixed labels, and we can fix any scripts and update existing issue labels as needed later? It should be very easy to update any issues with label "java" to have a label "Component: Java", for example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have implemented the suggestion. The workflow will check that the issue contains at least a label prefixed Component:. If someone with permissions can add the labels for the languages I am happy to update the labeler.yml workflow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can certainly do that. Maybe prepare the PR first, so that can be merged directly after renaming the labels?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: #14762
I've created everything in github (issue, and new title format) but I am not sure if we can merge yet :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I am not sure if we can merge yet :)

We will need some PRs to test Alessandro's PR updating the merge script anyway ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we implement duplicate labels now for those component that already have non-prefixed labels, and we can fix any scripts and update existing issue labels as needed later? It should be very easy to update any issues with label "java" to have a label "Component: Java", for example.

@toddfarmer all labels have been renamed to match the "Component: .." for now (we can alter, after the issues migration, consider renaming them again if we want)

await github.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: pullRequestNumber,
body: ":warning: GitHub issue #" + issueID + " **has no labels in GitHub**, please add labels for components."
})
}
} else {
await github.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: pullRequestNumber,
body: ":x: GitHub issue #" + issueID + " could not be retrieved."
})
}
}

module.exports = async ({github, context}) => {
const pullRequestNumber = context.payload.number;
const title = context.payload.pull_request.title;
const jiraID = helpers.detectJIRAID(title);
if (jiraID) {
await verifyJIRAIssue(github, context, pullRequestNumber, jiraID);
const issue = helpers.detectIssue(title)
if (issue){
if (issue.kind == "jira") {
await verifyJIRAIssue(github, context, pullRequestNumber, issue.id);
} else if(issue.kind == "github") {
await verifyGitHubIssue(github, context, pullRequestNumber, issue.id);
}
}
};
56 changes: 51 additions & 5 deletions .github/workflows/dev_pr/link.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,16 @@
const helpers = require("./helpers.js");


async function haveComment(github, context, pullRequestNumber, body) {
/**
* Checks whether message is present on Pull Request list of comments.
*
* @param {Object} github
* @param {Object} context
* @param {String} pullRequestNumber
* @param {String} message
* @returns {Boolean} true if message was found.
*/
async function haveComment(github, context, pullRequestNumber, message) {
const options = {
owner: context.repo.owner,
repo: context.repo.repo,
Expand All @@ -27,7 +36,7 @@ async function haveComment(github, context, pullRequestNumber, body) {
};
while (true) {
const response = await github.issues.listComments(options);
if (response.data.some(comment => comment.body === body)) {
if (response.data.some(comment => comment.body === message)) {
return true;
}
if (!/;\s*rel="next"/.test(response.headers.link || "")) {
Expand All @@ -38,6 +47,14 @@ async function haveComment(github, context, pullRequestNumber, body) {
return false;
}

/**
* Adds a comment on the Pull Request linking the JIRA issue.
*
* @param {Object} github
* @param {Object} context
* @param {String} pullRequestNumber
* @param {String} jiraID
*/
async function commentJIRAURL(github, context, pullRequestNumber, jiraID) {
const jiraURL = `https://issues.apache.org/jira/browse/${jiraID}`;
if (await haveComment(github, context, pullRequestNumber, jiraURL)) {
Expand All @@ -51,11 +68,40 @@ async function commentJIRAURL(github, context, pullRequestNumber, jiraID) {
});
}

/**
* Adds a comment on the Pull Request linking the GitHub issue.
*
* @param {Object} github
* @param {Object} context
* @param {String} pullRequestNumber - String containing numeric id of PR
* @param {String} issueID - String containing numeric id of the github issue
*/
async function commentGitHubURL(github, context, pullRequestNumber, issueID) {
// Make the call to ensure issue exists before adding comment
const issueInfo = await helpers.getGitHubInfo(github, context, issueID, pullRequestNumber);
const message = "* Github Issue: #" + issueInfo.number
if (await haveComment(github, context, pullRequestNumber, message)) {
return;
}
if (issueInfo){
await github.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: pullRequestNumber,
body: message
});
}
}

module.exports = async ({github, context}) => {
const pullRequestNumber = context.payload.number;
const title = context.payload.pull_request.title;
const jiraID = helpers.detectJIRAID(title);
if (jiraID) {
await commentJIRAURL(github, context, pullRequestNumber, jiraID);
const issue = helpers.detectIssue(title);
if (issue){
if (issue.kind == "jira") {
await commentJIRAURL(github, context, pullRequestNumber, issue.id);
} else if (issue.kind == "github") {
await commentGitHubURL(github, context, pullRequestNumber, issue.id);
}
}
};
7 changes: 4 additions & 3 deletions .github/workflows/dev_pr/title_check.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
const fs = require("fs");
const helpers = require("./helpers.js");

async function commentOpenJIRAIssue(github, context, pullRequestNumber) {
async function commentOpenGitHubIssue(github, context, pullRequestNumber) {
const {data: comments} = await github.issues.listComments({
owner: context.repo.owner,
repo: context.repo.repo,
Expand All @@ -41,7 +41,8 @@ async function commentOpenJIRAIssue(github, context, pullRequestNumber) {
module.exports = async ({github, context}) => {
const pullRequestNumber = context.payload.number;
const title = context.payload.pull_request.title;
if (!helpers.haveJIRAID(title)) {
await commentOpenJIRAIssue(github, context, pullRequestNumber);
const issue = helpers.detectIssue(title)
if (!issue) {
await commentOpenGitHubIssue(github, context, pullRequestNumber);
}
};
13 changes: 9 additions & 4 deletions .github/workflows/dev_pr/title_check.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,23 @@

Thanks for opening a pull request!

If this is not a [minor PR](https://github.com/apache/arrow/blob/master/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW
If this is not a [minor PR](https://github.com/apache/arrow/blob/master/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening JIRAs ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.

Then could you also rename pull request title in the following format?
Then could you also rename the pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of old issues on JIRA the title also supports:

ARROW-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

* [Other pull requests](https://github.com/apache/arrow/pulls/)
Expand Down