Skip to content

build: do not close tracking issues #3106

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Nov 28, 2024
Merged

build: do not close tracking issues #3106

merged 6 commits into from
Nov 28, 2024

Conversation

gunjjoshi
Copy link
Member

@gunjjoshi gunjjoshi commented Nov 12, 2024 •
edited
Loading

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

Snehil-Shah
Snehil-Shah reviewed Nov 12, 2024
View reviewed changes
Copy link
Member

@Snehil-Shah Snehil-Shah left a comment •
edited
Loading

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 •
edited
Loading

@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 •
edited
Loading

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.

@kgryte kgryte requested a review from Planeshifter November 27, 2024 02:17
Planeshifter
Planeshifter approved these changes Nov 28, 2024
View reviewed changes
Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

Made a small refactoring to ensure the note is not added to the proposed commit message, but included in the stdlib-bot comment.

@Planeshifter Planeshifter added Ready To Merge A pull request which is ready to be merged. and removed Needs Discussion Needs further discussion. labels Nov 28, 2024
@stdlib-bot
Copy link
Contributor

stdlib-bot commented Nov 28, 2024 •
edited by Planeshifter
Loading

PR Commit Message

build: do not close tracking issues

PR-URL: https://github.com/stdlib-js/stdlib/pull/3106

Co-authored-by: Philipp Burckhardt <[email protected]>
Reviewed-by: Philipp Burckhardt <[email protected]>
Reviewed-by: Snehil Shah <[email protected]>

Please review the above commit message and make any necessary adjustments.

@Planeshifter Planeshifter merged commit 890af82 into stdlib-js:develop Nov 28, 2024
7 checks passed
@Planeshifter
Copy link
Member

Planeshifter commented Nov 28, 2024 •
edited
Loading

Landed. Thanks @gunjjoshi for suggesting this improvement and opening a PR for it!

Planeshifter added a commit that referenced this pull request May 20, 2025
@Planeshifter
build: ensure workflow does not fail for non-zero exit codes
18794bf 
#3106 introduced a non-zero exit
code for the `.github/workflows/scripts/generate_pr_commit_message`
script whenever the PR references a tracking issue. This caused the
workflow to fail for all such PRs due to GitHub Actions running shell
scripts using bash -e {0}, causing the script to exit immediately
if any command exits with a non-zero status.

---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
  - task: lint_filenames
    status: passed
  - task: lint_editorconfig
    status: passed
  - task: lint_markdown
    status: na
  - task: lint_package_json
    status: na
  - task: lint_repl_help
    status: na
  - task: lint_javascript_src
    status: na
  - task: lint_javascript_cli
    status: na
  - task: lint_javascript_examples
    status: na
  - task: lint_javascript_tests
    status: na
  - task: lint_javascript_benchmarks
    status: na
  - task: lint_python
    status: na
  - task: lint_r
    status: na
  - task: lint_c_src
    status: na
  - task: lint_c_examples
    status: na
  - task: lint_c_benchmarks
    status: na
  - task: lint_c_tests_fixtures
    status: na
  - task: lint_shell
    status: na
  - task: lint_typescript_declarations
    status: na
  - task: lint_typescript_tests
    status: na
  - task: lint_license_headers
    status: passed
---
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Planeshifter Planeshifter Planeshifter approved these changes

+1 more reviewer

@Snehil-Shah Snehil-Shah Snehil-Shah left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Ready To Merge A pull request which is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gunjjoshi @kgryte @Planeshifter @stdlib-bot @Snehil-Shah