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

docs: Update PR templates #7290

Merged
merged 10 commits into from
May 30, 2024
Merged

docs: Update PR templates #7290

merged 10 commits into from
May 30, 2024

Conversation

jbkyang-nvi
Copy link
Contributor

@jbkyang-nvi jbkyang-nvi commented May 28, 2024

What does the PR do?

This PR adds a guideline template that all PRs should follow, so that Triton maintains its code quality.
This will populate any new PRs with this description.
ref

Checklist

  • PR title reflects the change and is of format <commit_type>: <Title>
  • Changes are described in the pull request.
  • Related issues are referenced.
  • Populated github labels field
  • Added test plan and verified test passes.
  • Verified that the PR passes existing CI.
  • Verified copyright is correct on all changed files.
  • Added succinct git squash message before merging ref.
  • All template sections are filled out.
  • Optional: Additional screenshots for behavior/output changes with before/after.

Commit Type:

Check the conventional commit type
box here and add the label to the github PR.

  • build
  • chore
  • ci
  • docs
  • feat
  • fix
  • perf
  • refactor
  • revert
  • style
  • test

Related PRs:

No related PRs

Where should the reviewer start?

.github/PULL_REQUEST_TEMPLATE/pull_request_template.md

Test plan:

No e2e tests added, this page should test if it works

  • CI Pipeline ID: N/A

Internal contribution view: pull_request_template
external contribution view: pull_request_template_external_contrib.md

Caveats:

Reviewers are in charge of verifying the fields are all populated

Background

Currently Triton PRs have been getting merged with many lines of redundant git commits (not squashed), or have been neglected in testing. This is an effort to improve this behavior

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

None

@jbkyang-nvi jbkyang-nvi force-pushed the kyang-update-PR-template branch from d8553bb to 8d1147d Compare May 28, 2024 23:53
@jbkyang-nvi jbkyang-nvi self-assigned this May 28, 2024
GuanLuo
GuanLuo previously approved these changes May 29, 2024
Copy link
Contributor

@GuanLuo GuanLuo left a comment

Choose a reason for hiding this comment

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

MTGL. Left some comments for style.

.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
GuanLuo
GuanLuo previously approved these changes May 29, 2024
<!-- Describe your pull request here. Please read the text below the line, and make sure you follow the checklist.-->


#### Checklist
Copy link
Contributor

Choose a reason for hiding this comment

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

can you include checking/updating copyright years in this checklist?

@@ -0,0 +1,43 @@
#### What does the PR do?
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on:

  1. One template, adding an "External Contributor" section with a reminder of CLA requirement
    or
  2. Two templates: Internal and External Contributor? External one having CLA requirement as well has possibly fewer section like no Pipeline ID?

Copy link
Contributor

Choose a reason for hiding this comment

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

Aiming to reduce the overhead of external PRs having the extra round trip of "Hey, did you sign the CLA?"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think two templates make sense - as it also would could have a section on pre-commit / code ql


#### Type:
- [ ] Feature
- [ ] Bugfix
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just use the Conventional Commits list if that's what we're going for here?

[
  'build',
  'chore',
  'ci',
  'docs',
  'feat',
  'fix',
  'perf',
  'refactor',
  'revert',
  'style',
  'test'
];

https://github.com/conventional-changelog/commitlint/tree/master/%40commitlint/config-conventional

Copy link
Contributor

Choose a reason for hiding this comment

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

And do we want to enforce having it in the PR title for automatic release notes / change logs?

ex:

chore: Update PR Template

Copy link
Contributor

Choose a reason for hiding this comment

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

like the idea of referencing an existing standard / practice

Copy link
Contributor

@rmccorm4 rmccorm4 May 29, 2024

Choose a reason for hiding this comment

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

It looks like there are some github actions that can do either/both:

  1. enforce PR titles follow the structure <conventional_commit_type>: <Title>
  2. enforce all commit messages follow <conventional_commit_type>: <commit_message>

I think (2) may be overkill or more of a "shock" to current workflows, but (1) could be a nice start for automating the check - maybe consider (2) later IMO.

I didn't find any particular action/tool/repo that seemed like more of a gold standard than the others, so may need to poke around at what some other big projects do that we like.

@matthewkotila
Copy link
Contributor

In general, looks good to me--thanks for helping us improve our code quality 🙏

@nnshah1 nnshah1 self-requested a review May 29, 2024 01:34
@jbkyang-nvi jbkyang-nvi changed the title Update PR template chore: Update PR templates May 29, 2024
@jbkyang-nvi jbkyang-nvi added the PR: build Changes that affect the build system or external dependencies label May 29, 2024
@jbkyang-nvi jbkyang-nvi added PR: docs Documentation only changes and removed PR: build Changes that affect the build system or external dependencies labels May 29, 2024
GuanLuo
GuanLuo previously approved these changes May 30, 2024
@jbkyang-nvi jbkyang-nvi changed the title chore: Update PR templates docs: Update PR templates May 30, 2024
nnshah1
nnshah1 previously approved these changes May 30, 2024
@jbkyang-nvi jbkyang-nvi dismissed stale reviews from nnshah1 and GuanLuo via d181eed May 30, 2024 17:48
@jbkyang-nvi jbkyang-nvi merged commit 353f24e into main May 30, 2024
3 checks passed
@jbkyang-nvi jbkyang-nvi deleted the kyang-update-PR-template branch May 30, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: docs Documentation only changes
Development

Successfully merging this pull request may close these issues.