-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
github: unify PR description and commit message #36205
Conversation
To improve the quality of commit messages in the revision history, indicate that the commit message will be part of the PR review process and integrate the WHAT and WHY questions in the change explanation. Remove the checklist item for PR title and description as it becomes redundant with this new text.
Pinging @elastic/security-external-integrations (Team:Security-External Integrations) |
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
|
||
<!-- Mandatory | ||
Explain here the changes you made on the PR. Please explain the WHAT: patterns used, algorithms implemented, design architecture, message processing, etc. | ||
--> | ||
Explain here the changes you made on the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove this one as we already have "Please explain:" just after.
Otherwise everything is good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is a matter of style. The text here reflects the text that was previously there, just now on two lines. The thinking that I had with retaining the text as it is was that the first line is the overall goal of the section, while the second is the specific parts that should be addressed. If you still want me to remove it given that, I'm happy to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a problem with it as it is, but if you can make it shorter people are more likely to read it :p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree about that. We need to have reviewers on board to point to this, so the main issue is whether that will happen.
To improve the quality of commit messages in the revision history, indicate that the commit message will be part of the PR review process and integrate the WHAT and WHY questions in the change explanation. Remove the checklist item for PR title and description as it becomes redundant with this new text.
What does this PR do?
To improve the quality of commit messages in the revision history, indicate that the commit message will be part of the PR review process and integrate the WHAT and WHY questions in the change explanation.
Remove the checklist item for PR title and description as it becomes redundant with this new text.
Why is it important?
Included in the text above.
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs