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

build: do not close tracking issues #3106

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

gunjjoshi
Copy link
Member

@gunjjoshi gunjjoshi commented Nov 12, 2024

Description

What is the purpose of this pull request?

This pull request:

  • modifies the generate_pr_commit_message script to not close tracking issues.
  • checks for (tracking issue) in the issue title, or a Tracking issue label.

Related Issues

Does this pull request have any related issues?

None.

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

Copy link
Member

@Snehil-Shah Snehil-Shah left a comment

Choose a reason for hiding this comment

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

If I'm not wrong, this just changes the final commit message right? GitHub automatically closes linked issues (linked using keywords like "Resolves") when a pull request is merged (ref).

Maybe we can follow the GitHub flow and create sub-issues (a new feature ig). Each PR resolves a sub issue. This will also fix the problem of manually having to update a list of checkboxes.

@kgryte
Copy link
Member

kgryte commented Nov 12, 2024

@gunjjoshi Thanks for opening this PR; however, there may be legitimate instances where we do want a commit/PR to close a tracking issue.

I think the actual workaround is that @Planeshifter needs to be more diligent about not closing tracking issues and first checking the linked issue(s) in the OP before further review/merge. This has been a too frequent occurrence recently.

In general, verifying linked issues is on the reviewer. For example, it is fairly common for me to manually update an OP to ensure that certain issues are not closed.

@kgryte kgryte added the Needs Discussion Needs further discussion. label Nov 12, 2024
@gunjjoshi
Copy link
Member Author

If I'm not wrong, this just changes the final commit message right? GitHub automatically closes linked issues (linked using keywords like "Resolves") when a pull request is merged (ref).

Maybe we can follow the GitHub flow and create sub-issues (a new feature ig). Each PR resolves a sub issue. This will also fix the problem of manually having to update a list of checkboxes.

That could be possible. Also, as we already have tracking issues setup in place, I believe we can think of sub-issues when we create new tracking issues in the future.

@gunjjoshi
Copy link
Member Author

gunjjoshi commented Nov 13, 2024

@kgryte Thanks for the suggestions. We might require closing the tracking issue only a single time, while the completion of sub-tasks would be more often. For this, can we add a custom keyword (different from ones such as "Closes", "Resolves", etc)? We can add that custom keyword in the OP, if the PR completes the last task in the tracking issue. If that custom keyword is mentioned in the OP, we can then add "Closes" in the commit message.

But I am not sure if we should do this.

@Planeshifter
Copy link
Member

Planeshifter commented Nov 13, 2024

We can investigate using sub-issues going forward, but this PR will still be helpful in the meantime.
However, Snehil raises an important point: if the PR post contains a "Resolves #", then even merging the PR with a different commit message still auto-closes the tracking issue.
In light of this, it would be good if we could add a sentence to the posted comment that informs the reviewer of the tracing issue and instructs them to edit the opening PR post.
Would you be able to add that please, @gunjjoshi?
Can you also change the current behavior to only check for the label and not the issue title? @kgryte and I discussed the PR quickly and landed on that as a cleaner alternative.

Last but not least, it would be good for us to update the pull request template, which currently has a "Resolves #" placeholder, with a comment on how to reference issues properly depending on the circumstances.

@gunjjoshi
Copy link
Member Author

@Planeshifter Sure, I'll update this PR. In conclusion, we will check if the issue has a Tracking issue label. If yes, the commit message will contain a comment, suggesting the reviewer to modify the OP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Discussion Needs further discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants