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

Set up default owners and the pull request checklist #270

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

psss
Copy link
Collaborator

@psss psss commented Dec 4, 2024

Let's enable the CODEOWNERS file so that we do not have to ask for review manually for each pull request. Also add the pull request checklist so that we don't forget about the important steps (like including the release note which was missing in all recent pulls).

@psss psss added this to the 1.6 milestone Dec 4, 2024
@psss
Copy link
Collaborator Author

psss commented Dec 4, 2024

@lukaszachy, @happz, @thrix, @janhavlin, @martinhoyer, are you ok with being included in the default list of reviewers?
@LecrisUT, would you like to be added as well?

Copy link
Contributor

@martinhoyer martinhoyer left a comment

Choose a reason for hiding this comment

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

I wonder if we could try to make the checklist "expandable". Is this how it's done?

<details>
  <summary>Checklist</summary>

- [ ] Implement the feature
- [ ] Write the documentation
- [ ] Extend the test coverage
- [ ] Mention the version
- [ ] Include a release note

</details>

Oh and maybe add

## Description
<!-- Briefly describe what this PR does and why it is needed. -->

## Related Issues
<!-- List any related issues or tasks. Use keywords like "Fixes #123" to close them automatically. -->
- Fixes #

Before or after the checklist so it's at somehow standardized? I ne er know if the checklist should stay at the top or go at the bottom for example :) (would vote to have it at the bottom)

@LecrisUT
Copy link
Contributor

LecrisUT commented Dec 4, 2024

@LecrisUT, would you like to be added as well?

No need I have this repo on watch list

I wonder if we could try to make the checklist "expandable". Is this how it's done?

I would vote to keep it outside expandable environment because it's something that would be updated regularly and should be checked by both author and reviewer.

👍 for the sections separator

@psss
Copy link
Collaborator Author

psss commented Dec 9, 2024

I would vote to keep it outside expandable environment because it's something that would be updated regularly and should be checked by both author and reviewer.

Agree with this. The main point of the check list is to keep these important steps in sight so that we don't forget them. Hiding the checklist under collapsable section would not help with this plus additional clicks would be needed for each update.

psss added 2 commits December 9, 2024 09:24
Let's enable the `CODEOWNERS` file so that we do not have to ask for
review manually for each pull request. Also add the pull request
checklist so that we don't forget about the important steps (like
including the release note which was missing in all recent pulls).
@psss psss force-pushed the checklist-and-owners branch from b0f8c7b to 8bfe822 Compare December 9, 2024 08:25
@psss
Copy link
Collaborator Author

psss commented Dec 9, 2024

Oh and maybe add

## Description
<!-- Briefly describe what this PR does and why it is needed. -->

## Related Issues
<!-- List any related issues or tasks. Use keywords like "Fixes #123" to close them automatically. -->
- Fixes #

Before or after the checklist so it's at somehow standardized? I ne er know if the checklist should stay at the top or go at the bottom for example :) (would vote to have it at the bottom)

I'm not sure about this. For me a more efficient workflow is to include the description in the commit details when writing the commit message. This get's automatically synced to the pull request description plus you get the checklist automatically appended (at the bottom). So, if user includes Fix #12345 directly in the commit message, creating a new pull request is just one click. No copy-and-pasting content into the right sections.

Using section separator for the checklist sounds good, added in 8bfe822.

@LecrisUT
Copy link
Contributor

LecrisUT commented Dec 9, 2024

I'm not sure about this. For me a more efficient workflow is to include the description in the commit details when writing the commit message. This get's automatically synced to the pull request description plus you get the checklist automatically appended (at the bottom).

It depends on repo settings. It can be configured to either use the PR's title and description, or the commit messages (don't remember if only title or full message). Allowing to include the Fixes in the description is a good fallback to make sure it links to the issue, and GH will display it on the right side in the "Development" section. It is also generally more intuitive for the drive-by contributor to use that instead of commits. How about instead indicating that you may add it in either the PR description or the commits.

@psss
Copy link
Collaborator Author

psss commented Dec 9, 2024

Allowing to include the Fixes in the description is a good fallback to make sure it links to the issue, and GH will display it on the right side in the "Development" section.

Yes, mentioning the fixed or releated issue is definitely a good idea. And can be done in both commit details description or later in the pull request description. We also recommend this on the tmt contribute page. Perhaps we could mention it in the fmf docs as well. Or, maybe even link the tmt pull request recommendations as they are basically the same.

@psss psss self-assigned this Dec 9, 2024
@psss psss merged commit 95c9745 into main Dec 9, 2024
12 of 13 checks passed
@psss psss deleted the checklist-and-owners branch December 9, 2024 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants