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

feat(ci): add PR title check workflow #12340

Merged
merged 12 commits into from
Aug 9, 2024
Merged

feat(ci): add PR title check workflow #12340

merged 12 commits into from
Aug 9, 2024

Conversation

rjan90
Copy link
Contributor

@rjan90 rjan90 commented Aug 5, 2024

Related Issues

Closes: #12148

Proposed Changes

  • A GitHub Actions workflow to automatically check and enforce a consistent format for PR titles
  • Started work on CONTRIBUTING.md doc that consolidates contributing expectations in one place. Establish CONTRIBUTING.md #12360

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • Update CHANGELOG.md or signal that this change does not need it.
    • If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
    • If the change does not require a CHANGELOG.md entry, do one of the following:
      • Add [skip changelog] to the PR title
      • Add the label skip/changelog to the PR
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

Add PR title check workflow

Add colon

Add colon

Request change to PR title instead of blocking PR

Request change to PR title instead of blocking PR

Remove change requested if title conforms to the formate

Remove change requested if title conforms to the formate
fix: workflow: Ensure PR title check posts review comments
@rjan90 rjan90 changed the title Add PR title check workflow Test Add PR title check workflow Aug 5, 2024
Add fail if title is invalid
@rjan90 rjan90 changed the title Test Add PR title check workflow Add PR title check workflow Aug 5, 2024
fix: workflow: Ensure PR title check fails for invalid titles
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Please update the PR title to match the required format: <type>: <scope>: <subject>

Types: feat, fix, docs, style, refactor, test, chore
Scope: required
Subject: start with lowercase

@rjan90 rjan90 changed the title Add PR title check workflow feat: ci: Add PR title check workflow Aug 5, 2024
@github-actions github-actions bot dismissed their stale review August 5, 2024 13:21

PR title now matches the required format.

refactor: pr-title-check: Streamline workflow and reduce complexity
@rjan90 rjan90 changed the title feat: ci: Add PR title check workflow Add PR title check workflow - check that it fails Aug 5, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Please update the PR title to match the required format: <type>: <scope>: <subject>

Types: feat, fix, docs, style, refactor, test, chore
Scope: required
Subject: start with lowercase

@rjan90
Copy link
Contributor Author

rjan90 commented Aug 5, 2024

Create a PR with an invalid title format and verify that it's blocked

Confirm that creating a PR with a invalid title format is being requested for change by the GA bot, as per this comment.

@rjan90 rjan90 changed the title Add PR title check workflow - check that it fails feat: ci: Add PR title check workflow Aug 5, 2024
@github-actions github-actions bot dismissed their stale review August 5, 2024 13:30

PR title now matches the required format.

@rjan90
Copy link
Contributor Author

rjan90 commented Aug 5, 2024

Edit a PR title from invalid to valid format and confirm the check passes
Confirm that "Changes requested" gets removed once updated to the correct format.

After updating my title to valid format, I can confirm that the check passes, and also that the "Changes requested" by the bot gets dismissed:
Screenshot 2024-08-05 at 15 30 12

@rjan90 rjan90 added the skip/changelog This change does not require CHANGELOG.md update label Aug 5, 2024
@rjan90 rjan90 requested a review from BigLep August 5, 2024 13:50
@rjan90 rjan90 marked this pull request as ready for review August 5, 2024 13:51
Update allowed PR-types pattern
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Please update the PR title to match the required format: <type>: <area>: <subject>

Types: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test, chore
Area: e.g. api, chain, state, mempool, multisig, networking, paych, proving, sealing, wallet, deps
Subject: start with lowercase

@rjan90 rjan90 changed the title feat: ci: Add PR title check workflow feat: ci: add PR title check workflow Aug 6, 2024
@github-actions github-actions bot dismissed their stale review August 6, 2024 06:36

PR title now matches the required format.

Copy link
Member

@BigLep BigLep left a comment

Choose a reason for hiding this comment

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

A few thoughts:

  1. (raised inline) Any reason we're not just following https://www.conventionalcommits.org/en/v1.0.0/ style?
  2. Do we expect to want to roll this out to other FilOz-maintained repos? I assume IPDX has ideas on how this could be done (asked here).
  3. What are the thoughts on using a 3rd party action? I realize there's not a lot of code here so it's not a big gain, but others have solved this problem. Examples:

.github/workflows/pr-title-check.yml Outdated Show resolved Hide resolved
.github/workflows/pr-title-check.yml Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Aug 8, 2024

fwiw I prefer conventional commit style and use it everywhere else, this is the repo that stands out as different in the world of repos I contribute to; but change can be hard for people that have got used to it

One option is to do both for a while and have a migration period.

^(feat|fix|build|chore|ci|docs|perf|refactor|revert|style|test)(\(\w+\))?:\s\w+:\s[a-z].+|^(feat|fix|build|chore|ci|docs|perf|refactor|revert|style|test)(\(\w+\))?:\s[a-z].+

@BigLep
Copy link
Member

BigLep commented Aug 8, 2024

migration period idea

Seems good/reasonable to me. We could still update our PR template text to steer at the conventional commit style.

Also, I'm realizing we'll need to update the regex to account for [skip changelog]

@rjan90
Copy link
Contributor Author

rjan90 commented Aug 8, 2024

  1. I agree that moving to the conventional commit style is preferred, and we should have a migration period where both styles are acceptable.

  2. I think it would be beneficial to roll this out to the repositories with higher activity and importance (builtin-actors, filecoin-ffi, ref-fvm, go-state-types, go-f3). For many of the smaller repositories with infrequent or less activity, I'm not sure if the benefits of such a title check outweigh the additional work (and potentially cost). However, I'm open-minded if there is an easy way to roll this out to all repositories maintained by FilOz.

  3. I will look into those 3rd party actions. Perhaps we can switch to one of these after we have completed the migration period here in the Lotus repo.

I will adjust the PR accordignly

@rjan90
Copy link
Contributor Author

rjan90 commented Aug 8, 2024

In 2cf80e7, I have changed:

  • Allow optional "[skip changelog]" prefix
  • Support both <type>: <scope>: <subject> and <type>(<scope>): <subject> formats
  • Use conventional commit <scope> instead of <area>
  • Alphabetized the Types and Scope in the body.

@rjan90
Copy link
Contributor Author

rjan90 commented Aug 8, 2024

While we are discussing conventions, referring to: Conventional Commits.

The commit contains the following structural elements, to communicate intent to the consumers of your library:
.............
3. BREAKING CHANGE: a commit that has a footer BREAKING CHANGE:, or appends a ! after the type/scope, introduces a breaking API change (correlating with MAJOR in Semantic Versioning). A BREAKING CHANGE can be part of commits of any type.

While this convention is primarily for commits (which Rod also has advocated for following: #12238 (comment)), it would be valuable to extend it to PR titles as well, particularly as we default to "squash and merge". By appending a ! after the in the PR title, we can clearly signal that the change is breaking.

Adjust pull_request_template.md according to the changes introduced with the PR title check workflow
@rjan90 rjan90 removed the skip/changelog This change does not require CHANGELOG.md update label Aug 8, 2024
@rjan90 rjan90 changed the title feat: ci: add PR title check workflow [skip changelg] feat: ci: add PR title check workflow Aug 8, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Please update the PR title to match one of the required formats:

  1. <type>: <scope>: <subject>
  2. <type>(<scope>): <subject>

Types: build, chore, ci, docs, feat, fix, perf, refactor, revert, style, test
Area/Scope: api, chain, state, mempool, multisig, networking, paych, proving, sealing, wallet, deps
Subject: start with lowercase

Optionally, you can add "[skip changelog]" at the start of the title if the change does not require a CHANGELOG.md entry.

@rjan90 rjan90 changed the title [skip changelg] feat: ci: add PR title check workflow [skip changelog] feat (ci): add PR title check workflow Aug 8, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Please update the PR title to match one of the required formats:

  1. <type>: <scope>: <subject>
  2. <type>(<scope>): <subject>

Types: build, chore, ci, docs, feat, fix, perf, refactor, revert, style, test
Area/Scope: api, chain, state, mempool, multisig, networking, paych, proving, sealing, wallet, deps
Subject: start with lowercase

Optionally, you can add "[skip changelog]" at the start of the title if the change does not require a CHANGELOG.md entry.

@rjan90 rjan90 changed the title [skip changelog] feat (ci): add PR title check workflow [skip changelog] feat(ci): add PR title check workflow Aug 8, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Please update the PR title to match one of the required formats:

  1. <type>: <scope>: <subject>
  2. <type>(<scope>): <subject>

Types: build, chore, ci, docs, feat, fix, perf, refactor, revert, style, test
Area/Scope: api, chain, state, mempool, multisig, networking, paych, proving, sealing, wallet, deps
Subject: start with lowercase

Optionally, you can add "[skip changelog]" at the start of the title if the change does not require a CHANGELOG.md entry.

@rjan90 rjan90 changed the title [skip changelog] feat(ci): add PR title check workflow [skip changelog] feat: ci: add PR title check workflow Aug 8, 2024
@github-actions github-actions bot dismissed their stale review August 8, 2024 12:48

PR title now matches the required format.

@rjan90 rjan90 changed the title [skip changelog] feat: ci: add PR title check workflow feat(ci): add PR title check workflow Aug 8, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Please update the PR title to match one of the required formats:

  1. <type>: <scope>: <subject>
  2. <type>(<scope>): <subject>

Types: build, chore, ci, docs, feat, fix, perf, refactor, revert, style, test
Area/Scope: api, chain, state, mempool, multisig, networking, paych, proving, sealing, wallet, deps
Subject: start with lowercase

Optionally, you can add "[skip changelog]" at the start of the title if the change does not require a CHANGELOG.md entry.

fix(ci): update regex to support both PR title formats
@github-actions github-actions bot dismissed their stale review August 8, 2024 13:08

PR title now matches the required format.

@rjan90 rjan90 dismissed github-actions[bot]’s stale review August 8, 2024 13:27

This is a stale review.

BigLep added a commit that referenced this pull request Aug 8, 2024
Copy link
Member

@BigLep BigLep left a comment

Choose a reason for hiding this comment

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

I'm good with this getting merged once feedback incorporated.

I created another PR on top of your PR that starts extracting out some of these contributing conventions as part of #12360

.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/workflows/pr-title-check.yml Outdated Show resolved Hide resolved
…12361)

This starts on #12360 as part of landing #12340

* Update .github/workflows/pr-title-check.yml

Co-authored-by: Phi-rjan <[email protected]>

* Update CONTRIBUTING.md

Co-authored-by: Phi-rjan <[email protected]>

* Update CONTRIBUTING.md

Co-authored-by: Phi-rjan <[email protected]>

* Update CONTRIBUTING.md

Co-authored-by: Phi-rjan <[email protected]>

---------

Co-authored-by: Phi-rjan <[email protected]>
@github-actions github-actions bot dismissed their stale review August 8, 2024 21:43

PR title now matches the required format.

@rjan90 rjan90 merged commit 7530136 into master Aug 9, 2024
86 checks passed
@rjan90 rjan90 deleted the phi/PR-title-check branch August 9, 2024 08:49
rjan90 added a commit to hanabi1224/lotus that referenced this pull request Aug 12, 2024
* Add PR title check workflow

Add PR title check workflow

Add colon

Add colon

Request change to PR title instead of blocking PR

Request change to PR title instead of blocking PR

Remove change requested if title conforms to the formate

Remove change requested if title conforms to the formate

* fix: workflow: Ensure PR title check posts review comments

fix: workflow: Ensure PR title check posts review comments

* Add fail if title is invalid

Add fail if title is invalid

* fix: workflow: Ensure PR title check fails for invalid titles

fix: workflow: Ensure PR title check fails for invalid titles

* refactor: pr-title-check: Streamline workflow and reduce complexity

refactor: pr-title-check: Streamline workflow and reduce complexity

* Update allowed PR-types pattern

Update allowed PR-types pattern

* Update pr-title-check.yml

Co-authored-by: Rod Vagg <[email protected]>

* Use conventional commit, but allow for legacy

- Allow optional "[skip changelog]" prefix
- Both <type>: <scope>: and <type>(<scope>): formats
- Use conventional commit "scope" instead of area
- Alphabetize <type> and <scope>

* docs: update pull_request_template.md

Adjust pull_request_template.md according to the changes introduced with the PR title check workflow

* fix(ci): update regex to support both PR title formats

fix(ci): update regex to support both PR title formats

* docs: seed CONTRIBUTING.md with pr title and changelog expectations (filecoin-project#12361)

This starts on filecoin-project#12360 as part of landing filecoin-project#12340

* Update .github/workflows/pr-title-check.yml

Co-authored-by: Phi-rjan <[email protected]>

* Update CONTRIBUTING.md

Co-authored-by: Phi-rjan <[email protected]>

* Update CONTRIBUTING.md

Co-authored-by: Phi-rjan <[email protected]>

* Update CONTRIBUTING.md

Co-authored-by: Phi-rjan <[email protected]>

---------

Co-authored-by: Phi-rjan <[email protected]>

* Update .github/workflows/pr-title-check.yml

Co-authored-by: Steve Loeppky <[email protected]>

---------

Co-authored-by: Rod Vagg <[email protected]>
Co-authored-by: Steve Loeppky <[email protected]>
rjan90 added a commit that referenced this pull request Aug 12, 2024
* Add PR title check workflow

Add PR title check workflow

Add colon

Add colon

Request change to PR title instead of blocking PR

Request change to PR title instead of blocking PR

Remove change requested if title conforms to the formate

Remove change requested if title conforms to the formate

* fix: workflow: Ensure PR title check posts review comments

fix: workflow: Ensure PR title check posts review comments

* Add fail if title is invalid

Add fail if title is invalid

* fix: workflow: Ensure PR title check fails for invalid titles

fix: workflow: Ensure PR title check fails for invalid titles

* refactor: pr-title-check: Streamline workflow and reduce complexity

refactor: pr-title-check: Streamline workflow and reduce complexity

* Update allowed PR-types pattern

Update allowed PR-types pattern

* Update pr-title-check.yml

Co-authored-by: Rod Vagg <[email protected]>

* Use conventional commit, but allow for legacy

- Allow optional "[skip changelog]" prefix
- Both <type>: <scope>: and <type>(<scope>): formats
- Use conventional commit "scope" instead of area
- Alphabetize <type> and <scope>

* docs: update pull_request_template.md

Adjust pull_request_template.md according to the changes introduced with the PR title check workflow

* fix(ci): update regex to support both PR title formats

fix(ci): update regex to support both PR title formats

* docs: seed CONTRIBUTING.md with pr title and changelog expectations (#12361)

This starts on #12360 as part of landing #12340

* Update .github/workflows/pr-title-check.yml

Co-authored-by: Phi-rjan <[email protected]>

* Update CONTRIBUTING.md

Co-authored-by: Phi-rjan <[email protected]>

* Update CONTRIBUTING.md

Co-authored-by: Phi-rjan <[email protected]>

* Update CONTRIBUTING.md

Co-authored-by: Phi-rjan <[email protected]>

---------

Co-authored-by: Phi-rjan <[email protected]>

* Update .github/workflows/pr-title-check.yml

Co-authored-by: Steve Loeppky <[email protected]>

---------

Co-authored-by: Rod Vagg <[email protected]>
Co-authored-by: Steve Loeppky <[email protected]>
@BigLep
Copy link
Member

BigLep commented Aug 12, 2024

For the record, concerning using off-the-shelf tools given we have had to iterate on this a couple times (e.g., PR #12373) , I did a little more researching. I don't think they'll give us fully what we want and we should finish fine-tuning what we started here:

Tool Has regex support? Post customizable comment message? Supports forks (pull_request_target)?
https:/marketplace/actions/pr-title-checker Yes Doesn't comment. It applies a label. No way for us to get a message that links to our conventions. (Didn't look)
https:/marketplace/actions/pr-title-verify Yes Not sure. Not documented. Not sure. Not documented.
https:/Slashgear/action-check-pr-title Yes Yes No

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

[DX Streamline] Add mechanism so PR titles conform to repo standard
3 participants