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

[Feature Request] Remove or shorten the PR checklist #13965

Closed
andrross opened this issue Jun 4, 2024 · 9 comments · Fixed by #13979
Closed

[Feature Request] Remove or shorten the PR checklist #13965

andrross opened this issue Jun 4, 2024 · 9 comments · Fixed by #13979
Labels
enhancement Enhancement or improvement to existing feature or request Other

Comments

@andrross
Copy link
Member

andrross commented Jun 4, 2024

Is your feature request related to a problem? Please describe

I personally find the PR checklist to be rote busy work that doesn't add a lot of value. Going entry by entry:

  • New functionality includes testing - This is general software development best practice. Reviewers should be enforcing this. We have code coverage metrics published on each PR as well. I'm skeptical this checklist entry is helpful.
  • All tests pass - PR workflows enforce this.
  • New functionality has been documented - The javadoc targets enforce this as a part of the build if this refers to javadoc. If not, then this is duplicated by the "public documentation" entry below.
  • New functionality has javadoc added - Same as above
  • API changes companion pull request created - I could be convinced this one should stay, maybe.
  • Failing checks are inspected and point to the corresponding known issue(s) - The failing checks post a comment linking to the flaky test guide telling you want to do.
  • Commits are signed per the DCO using --signoff - The DCO workflow enforces this
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog) - The changelog workflow enforces this.
  • Public documentation issue/PR created - I could be convinced this one should stay, maybe.

Describe the solution you'd like

Remove all checklist items except the "API Changes" and "Public Documentation" entries. Maybe consider removing the entire checklist if we don't have evidence the remaining two entries are effective.

Related component

Other

@andrross andrross added enhancement Enhancement or improvement to existing feature or request untriaged labels Jun 4, 2024
@github-actions github-actions bot added the Other label Jun 4, 2024
@kotwanikunal
Copy link
Member

+1 on the direction.
Ever since we have made all checks as required, items on the checklist are redundant.

@kotwanikunal
Copy link
Member

@peternied / @dblock - Thoughts here?

@andrross
Copy link
Member Author

andrross commented Jun 4, 2024

One potential goal of the checklist is to encourage contributors to do these things before submitting a PR. However, given how many PRs I see where none of the template or checklist is filled out, I'd say the checklist has proven to be ineffective at accomplishing this goal.

@peternied
Copy link
Member

I think of the checklist as a tool to inform on the requirements of the project and help remind maintainers what to check in on before merging a change. However, if we feel its working counter to this purpose or hindering contributions we can certainly change it.

If you were to ask me what I would target first, I wouldn't mind the removal of the sub-bullets All tests pass, New functionality has javadoc added - want to make a PR to start trimming them down?

@peternied
Copy link
Member

However, given how many PRs I see where none of the template or checklist is filled out, I'd say the checklist has proven to be ineffective at accomplishing this goal.

@andrross I see you point; however, I'd offer an alternative perspective when I see empty PR templates its a great indicator to provide guidance to that contributor.

@reta
Copy link
Collaborator

reta commented Jun 4, 2024

@andrross thanks a lot for bringing this one up, I think keeping the checklist but making it shorter would be a good path forward:

The functionality include testing
API changes companion pull request created
Public documentation companion pull request [created] created

WDYT?

@andrross
Copy link
Member Author

andrross commented Jun 4, 2024

@reta I like it.

My remaining nitpick is that the majority of PRs do not change the REST API nor require an update to the documentation website. Is there a way to still surface those concerns when applicable without forcing every PR to check a box?

@reta
Copy link
Collaborator

reta commented Jun 4, 2024

@reta I like it.

❤️

My remaining nitpick is that the majority of PRs do not change the REST API nor require an update to the documentation website. Is there a way to still surface those concerns when applicable without forcing every PR to check a box?

I think it is hard to have a template that is equally helpful for new and existing contributors. These two checklist items basically hint people that this is not all-in-one repository but there are at least 2 others one may need to look at. But keeping it short is less annoying for the seasoned contributors, nonetheless raising the awareness for others.

andrross added a commit to andrross/OpenSearch that referenced this issue Jun 4, 2024
This commit removes items from the checklist that were redundant in
order to reduce the work required to submit a PR.

Related opensearch-project#13965

Signed-off-by: Andrew Ross <[email protected]>
@peternied
Copy link
Member

[Triage - attendees 1 2 3 4 5 6 7]
@andrross Thanks for creating this issue, looking forward to a pull request

andrross added a commit to andrross/OpenSearch that referenced this issue Jun 5, 2024
This commit removes items from the checklist that were redundant in
order to reduce the work required to submit a PR. This also removes the
check that enforces checklist completion as requested by issue opensearch-project#10970.

Resolves opensearch-project#13965
Resolves opensearch-project#10970

Signed-off-by: Andrew Ross <[email protected]>
andrross added a commit that referenced this issue Jun 5, 2024
This commit removes items from the checklist that were redundant in
order to reduce the work required to submit a PR. This also removes the
check that enforces checklist completion as requested by issue #10970.

Resolves #13965
Resolves #10970

Signed-off-by: Andrew Ross <[email protected]>
andrross added a commit to andrross/OpenSearch that referenced this issue Jun 5, 2024
This commit removes items from the checklist that were redundant in
order to reduce the work required to submit a PR. This also removes the
check that enforces checklist completion as requested by issue opensearch-project#10970.

Resolves opensearch-project#13965
Resolves opensearch-project#10970

Signed-off-by: Andrew Ross <[email protected]>
(cherry picked from commit ac78f0a)
andrross added a commit to andrross/OpenSearch that referenced this issue Jun 5, 2024
This commit removes items from the checklist that were redundant in
order to reduce the work required to submit a PR. This also removes the
check that enforces checklist completion as requested by issue opensearch-project#10970.

Resolves opensearch-project#13965
Resolves opensearch-project#10970

Signed-off-by: Andrew Ross <[email protected]>
(cherry picked from commit ac78f0a)
reta pushed a commit that referenced this issue Jun 5, 2024
This commit removes items from the checklist that were redundant in
order to reduce the work required to submit a PR. This also removes the
check that enforces checklist completion as requested by issue #10970.

Resolves #13965
Resolves #10970

Signed-off-by: Andrew Ross <[email protected]>
(cherry picked from commit ac78f0a)
LantaoJin pushed a commit to LantaoJin/OpenSearch that referenced this issue Jun 6, 2024
This commit removes items from the checklist that were redundant in
order to reduce the work required to submit a PR. This also removes the
check that enforces checklist completion as requested by issue opensearch-project#10970.

Resolves opensearch-project#13965
Resolves opensearch-project#10970

Signed-off-by: Andrew Ross <[email protected]>
parv0201 pushed a commit to parv0201/OpenSearch that referenced this issue Jun 10, 2024
This commit removes items from the checklist that were redundant in
order to reduce the work required to submit a PR. This also removes the
check that enforces checklist completion as requested by issue opensearch-project#10970.

Resolves opensearch-project#13965
Resolves opensearch-project#10970

Signed-off-by: Andrew Ross <[email protected]>
kkewwei pushed a commit to kkewwei/OpenSearch that referenced this issue Jul 24, 2024
…project#14009)

This commit removes items from the checklist that were redundant in
order to reduce the work required to submit a PR. This also removes the
check that enforces checklist completion as requested by issue opensearch-project#10970.

Resolves opensearch-project#13965
Resolves opensearch-project#10970

Signed-off-by: Andrew Ross <[email protected]>
(cherry picked from commit ac78f0a)
Signed-off-by: kkewwei <[email protected]>
wdongyu pushed a commit to wdongyu/OpenSearch that referenced this issue Aug 22, 2024
This commit removes items from the checklist that were redundant in
order to reduce the work required to submit a PR. This also removes the
check that enforces checklist completion as requested by issue opensearch-project#10970.

Resolves opensearch-project#13965
Resolves opensearch-project#10970

Signed-off-by: Andrew Ross <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement or improvement to existing feature or request Other
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants