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

[Dev] Improve the link script to avoid extra comment #14790

Closed
amol- opened this issue Nov 30, 2022 · 8 comments · Fixed by #35811
Closed

[Dev] Improve the link script to avoid extra comment #14790

amol- opened this issue Nov 30, 2022 · 8 comments · Fixed by #35811

Comments

@amol-
Copy link
Member

amol- commented Nov 30, 2022

Describe the enhancement requested

Currently the link script used a dedicated comment to track that the link script had already run for that PR

https://github.com/apache/arrow/blob/master/.github/workflows/dev_pr/link.js#L86-L96

This is suboptimal because it leads to an extra comment, we could inspect the PR body/description itself for the "Closes: X" message instead of inspecting a comment.

Component(s)

Developer Tools

@assignUser
Copy link
Member

For reference here are the docs with all available keywords: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

close
closes
closed
fix
fixes
fixed
resolve
resolves
resolved

@jorisvandenbossche
Copy link
Member

@assignUser I don't think we have to check for all those? We explicitly want to check for the addition of the bot, which will always be * Closes: #xxx. Because I assume we still want to run the bot (since this also checks for other things) even if the user who opened the PR wrote a "fixes #.." in the top post.

@assignUser
Copy link
Member

Ah I see, makes sense. I thought we would also check if the user has already added the keyword but I guess that's not really necessary as doubling the keyword doesn't have any negative consequences

@amol-
Copy link
Member Author

amol- commented Dec 1, 2022

I never checked, but would issue_url or _links.issue be a good guard in this case? Given that when you add the "Closes: X" text GitHub links the PR to the ISsue, maybe we can simply check the value of those instead of having to parse the body.

@assignUser
Copy link
Member

Let me check the api docs, I know that there is no endpoint to link the issue but maybe that info is in the issue/pr object

@assignUser
Copy link
Member

There is no "linked_pr|issue" field or something in the issue object and there also is no endpoint to link issues and prs... seems to be handled in some hidden way in the backend?

@jorisvandenbossche
Copy link
Member

Also, if we would check for "issue linked" through github APIs, we would still need to check that the issue number is correct and matching with the title (because you can easily type something like "this might also fix#xxx" or "this does not fix #xxx" in your top post, which creates such a link on github unintentionally).

@amol-
Copy link
Member Author

amol- commented Dec 1, 2022

There is no "linked_pr|issue" field or something in the issue object and there also is no endpoint to link issues and prs... seems to be handled in some hidden way in the backend?

The PullRequest object has a issue_url and _links.issue don't those point to the linked issue?

raulcd added a commit that referenced this issue May 30, 2023
### Rationale for this change

We are duplicating the Closes issue_id comment on PRs adding it to both the PR body and a comment. There was some discussion to remove it from the comment.

### What changes are included in this PR?

Remove adding extra comment to PR and check whether `Closes XXX` was already added on the body instead of checking comment.

### Are these changes tested?

Yes, I have tested on my fork, see this PR: raulcd#79

### Are there any user-facing changes?

No
* Closes: #14790

Lead-authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
@raulcd raulcd added this to the 13.0.0 milestone May 30, 2023
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.

4 participants