ARROW-18380: [Dev] Update dev_pr GitHub workflows to accept both GitHub issues and JIRA #14731
Conversation
|
The PR expects the title to be either as it was before or: |
At the moment we can do this on the merge script as we do with JIRA. I don't want to automatically edit the PR body at the moment. We can tackle this later on. |
|
Apart from some refactoring based on the review comments, the main change from the examples on the description is that the issue will be automatically assigned to the PR creator if there are no assignees. See this comment on my fork as an example: raulcd#50 (comment) |
|
Looks good to me! I wanted to give the same comment as @assignUser that we could also update the top comment to include a link to the issue instead of adding it as a new comment (this could also be done for JIRAs, except that here it doesn't matter for auto-closing). But also agree this is fine to consider later, as that is a bigger change. |
| if (!issueInfo.assignees.length) { | ||
| await assignGitHubIssue(github, context, pullRequestNumber, issueInfo); | ||
| } | ||
| if(!issueInfo.labels.length) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I can certainly do that. Maybe prepare the PR first, so that can be merged directly after renaming the labels?
There was a problem hiding this comment.
Done: #14762
I've created everything in github (issue, and new title format) but I am not sure if we can merge yet :)
There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
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)
|
Benchmark runs are scheduled for baseline = fde7b93 and contender = b1bcd6f. b1bcd6f is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
This might require to be done once the merge_arrow_pr.sh script is also updated. Updating the merge PR script is tracked on GitHub for the migration here: #14720
I have tested this new workflow on my fork with the following PRs and issues on my fork.
Example of valid issue: raulcd#41
Example of issues without labels or assignees: raulcd#42 or raulcd#43
Example of issue with non existent GH issue: raulcd#45