Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MIGRATION: [Developer] The merge_arrow_pr script should be able to handle issues on GitHub and JIRA #14720

Closed
raulcd opened this issue Nov 23, 2022 · 7 comments · Fixed by #14750

Comments

@raulcd
Copy link
Member

raulcd commented Nov 23, 2022

Describe the feature requested.

As we are migrating from JIRA issues to GitHub issues and now that no new users can be created on JIRA we should modify the merge_pr script to be able to handle issues on GitHub.

I am happy to work on a PR but there are some things that might need to be decided first.

  1. How do we link issues with PRs? Currently we add the JIRA ticket id to the title of the PRs ARROW-XXXX:. Do we want to do the same with GitHub issues: GH-XXXXX: or do we want to have a mandatory Closes XXXX on the PR description so once the PR is merged the issue gets automatically closed? We have to define a way for either the merge script or the PR being merge to close the GitHub issue.
  2. We need to add Labels to GitHub to identify the different components. Currently the GitHub merge_pr shows the identified components when merging, we should keep that functionality.
  3. We should create the initial milestones so the merge_pr script is able to add, prompt the correct milestone when merging the PR.
  4. Do we want to add assignee to the issue as we do with JIRA?

The merge_pr script should continue working with JIRA as the parquet CPP project is not going to be move to GitHub as far as I understand.

Component

Developer Tools

@assignUser
Copy link
Member

  1. GitHub issues: GH-XXXXX ✖️ ✖️ The link between issue and PR should be made with the intended GH functionality, so either using a keyword like "closes ARROW-264: File format #123" or manually linking the PR and issue via the web gui. Adding the keyword to the PR body is basically the same as the ARROW-123 in the title, so replacing one with the other should be fine.
  2. We already have the labeler workflow that could easily be adapted to label components by the sub dirs of the changed files (e.g. r/* -> R cpp/* -> C++ dev/archery/* -> Developer dev/tasks -> CI ...)
  3. 👍
  4. For issues it would make sense to assign them when you start working on them. We could have the bot add the PR creator as an assignee on the matching issue on opening the PR. That would keep things consistent and also mark the issues as WIP? I don't think we need to use the assignee field on PRs themselves though.

I am unsure on the details of parquet as a "sub-project"(?) but I really don't think it makes any sense to use two separate tools for issue tracking within the same repo.

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Nov 24, 2022

2. We need to add Labels to GitHub to identify the different components. Currently the GitHub merge_pr shows the identified components when merging, we should keep that functionality.

This doesn't necessarily have to be in the merge script. For example, we could also have a bot that runs whenever a PR is merged, checks if there is a component label set, and if not adds a comment like "Hey merger, you forgot a component label!"

3. We should create the initial milestones so the merge_pr script is able to add, prompt the correct milestone when merging the PR.

I see that the 11.0.0 milestone already exists (the only relevant on the short term, I think, since for a potential 10.0.2 we still use JIRA?)

@pitrou
Copy link
Member

pitrou commented Nov 24, 2022

I like the GH-1234 convention. CPython does this and it generates hypertext links, so there must be a way to make it work:
python/cpython@8dbe08e

We would probably still use the merge script that would handle the duty of autoclosing the GH issue if desired.

@jorisvandenbossche
Copy link
Member

I think we should try to consistently use the GH-xxxx syntax to reference issues (that probably won't happen in general comments in discussions, but at least we could try to do this for the top-level comment that mentions the issue to close). That makes it clear it is a github issue, and is also more robust in case we have another migration in the future.

Whether we should include the "GH-xxx" in the title, I don't have a strong opinion (it makes it clearer that there it is a PR with an associated issue, but if we allow PRs without such associated issue, PRs would have a mix of titles with and without that prefix anyway ..)

@jorisvandenbossche
Copy link
Member

I like the GH-1234 convention. CPython does this and it generates hypertext links, so there must be a way to make it work:
python/cpython@8dbe08e

I think that's just GitHub that recognizes such text and make it into a hyperlink when rendering?
From that link, it seems that the commit message there is just the PR title + top-post body

@pitrou
Copy link
Member

pitrou commented Nov 24, 2022

I think that's just GitHub that recognizes such text and make it into a hyperlink when rendering?

Right, it just worked in your reply :-)

@raulcd raulcd self-assigned this Nov 25, 2022
@amol- amol- modified the milestone: 11.0.0 Nov 28, 2022
amol- pushed a commit that referenced this issue Nov 29, 2022
…ub issues and JIRA (#14731)

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

Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Alessandro Molina <amol@turbogears.org>
jorisvandenbossche pushed a commit that referenced this issue Dec 1, 2022
…14750)

* Closes: #14720

Lead-authored-by: Alessandro Molina <amol@turbogears.org>
Co-authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants