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

Bug Report: Linking PR and Issues are not closing them #702

Closed
Thomas-Boi opened this issue Jun 26, 2021 · 8 comments
Closed

Bug Report: Linking PR and Issues are not closing them #702

Thomas-Boi opened this issue Jun 26, 2021 · 8 comments
Labels
bug Use this label for pointing out bugs enhancement

Comments

@Thomas-Boi
Copy link
Member

Bug

Merging a linked PR into the master branch does not close its corresponding issue. See #592 and confirm the PR is merged into master using this url (just Ctrl + F and search for raspberry). Also a few others like #582 and #543.

How to replicate the bug

Make an issue and make a PR. Link those together and merge the PR into master. The Issue will probably not close.

Possible Fixes/Solutions

Not sure. Here's what we've tried:

  • Linking the issue using the GitHub website
  • Linking the issue by using keywords and mentioning the issue number in the comments (new icon: Qt (original) #691)
  • Searching on GitHub Community (found no issues similar to this)

My next move will be making a thread about this on the Community website. Hopefully, we'll get an answer there

@Thomas-Boi Thomas-Boi added the bug Use this label for pointing out bugs label Jun 26, 2021
@Thomas-Boi
Copy link
Member Author

After reading the docs one more time, I have a feeling that the PR should be merged directly into master to have the linked issue closed properly.

When you merge a linked pull request into the default branch of a repository, its linked issue is automatically closed. For more information about the default branch, see "Changing the default branch."

Technically, if I make PR number 123 and merge it into develop, the PR from develop merging into master will be a different one.

I can think of two ways to address this:

  • Changing the develop branch to be the default branch
  • In the PR from develop into master, we have to manually link the issues. I'll look into whether this is possible using the GitHub API

@Thomas-Boi
Copy link
Member Author

Thomas-Boi commented Jun 28, 2021

After further research, I'm more convinced the problem is we are not merging the PR into master. Even though later on, we will merge those commits in but we will need to manually link the issues at that time. Research link

Currently, the API doesn't provide a way to find linked issues using a PR number. Thus, our options are now:

  1. Changing the develop branch to be the default branch
  2. Manually close issues when we finished merging into develop
  3. Keep track of linked issues somehow and link them in the release PR from develop to master

What do you guys think?

EDIT:
Found a 4th option:
4. Make a new workflow that utilizes the pull_request trigger's closed activity and this action. If it's a merge and the base is develop, we will close the issue associated with the PR. This will achieve the behaviour that we want on a non-default branch.

@Thomas-Boi
Copy link
Member Author

Thomas-Boi commented Jun 28, 2021

I think I'm going ahead with option 4. I don't think we should make develop to be the default branch. The other ones are a lot of work => might as well make the action so we don't have to worry about it.

EDIT:
The current method, while work, will confuse users who only look at master. While develop will contain a feature/fix, master won't have it until we merge develop into master

  • To make things clear, we should only close an issue when it's properly merged into master.

I've revised the plan:

  • I'll still make a script that runs after a PR is merged into develop. However, rather than closing the linked issue, I'll label it instead, like fixed-in-develop for example.
  • Then, when we make a build release, another script will be triggered that will look for all issues with that label then close them.

Since this requires more moving parts, let me know what you think. I'll wait for other maintainers' thoughts before I continue.

EDIT: @amacado are you ok with this new workflow?

@amacado
Copy link
Member

amacado commented Jul 3, 2021

@Thomas-Boi sure, option 4 sounds like a great idea. I could think that many (or at least 'more than our') project has this problem. Have you tried finding an available, existing github action? If not maybe we can build one which is not exclusive for devicon?

@Thomas-Boi
Copy link
Member Author

I searched around for a bit more and found a few more options:

@Thomas-Boi
Copy link
Member Author

So good news: putting the Issue number in the commit will close the issue once it's merged into master. We can utilize this feature from now on.

However, we still need a way to differentiate between PRs that are in develop vs in master. Still, I'll close this Issue for now since our question does get addressed

@Snailedlt
Copy link
Collaborator

So good news: putting the Issue number in the commit will close the issue once it's merged into master. We can utilize this feature from now on.

However, we still need a way to differentiate between PRs that are in develop vs in master. Still, I'll close this Issue for now since our question does get addressed

Just to clarify this. The issue will only be closed if you link to the issue in the correct way, by using one of the following keywords followed by #issueNumber.

Example that closes issues with a commit, once it's merged into master:
git commit -m "closes #1, closes #2, closes #3; YOUR COMMIT MESSAGE" (this will close issues 1, 2 and 3).

Example that only references an issue, but doesn't close it:
git commit -m "#1, #2, #3; YOUR COMMIT MESSAGE" (This will reference issues 1, 2 and 3)

Read this for more info on referencing/closing issues through commits

@Thomas-Boi
Copy link
Member Author

Thank you for the tip @Snailedlt! I'm thinking what you said is the correct one since I remember testing it out with a PR once before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Use this label for pointing out bugs enhancement
Projects
None yet
Development

No branches or pull requests

3 participants