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

Check commits instead of PR text for pull requests #28

Closed
krahlro-sick opened this issue Jul 8, 2020 · 14 comments · Fixed by #122
Closed

Check commits instead of PR text for pull requests #28

krahlro-sick opened this issue Jul 8, 2020 · 14 comments · Fixed by #122
Labels
enhancement New feature or request

Comments

@krahlro-sick
Copy link
Contributor

The tool should check the commits that are part of a PR instead of the PR text if it is executed for a pull request (example run.

@krahlro-sick
Copy link
Contributor Author

As discussed via mail: It might make sense to also check the PR title if there are multiple commits as it is included in the squashed commit message (example).

@mristin
Copy link
Owner

mristin commented Jul 9, 2020

Please see: #30 (comment)

I'll add a clarification in the readme.

@mristin
Copy link
Owner

mristin commented Jul 9, 2020

The relevant section in the readme is here: https://github.com/mristin/opinionated-commit-message#known-issue

I'll leave this issue open to gauge how many people need this feature.

@mristin mristin added the enhancement New feature or request label Jul 9, 2020
mristin added a commit to admin-shell-io/aasx-package-explorer that referenced this issue Jul 9, 2020
This splits the commit message checks from the check styles into a
separate workflow which runs on every pull request and push.

The style checks were run only on pull request, hence ignoring the
commit messages of a branch in the pull request.

See also this issue of the checker:
mristin/opinionated-commit-message#28
mristin added a commit to admin-shell-io/aasx-package-explorer that referenced this issue Jul 9, 2020
This splits the commit message checks from the check styles into a
separate workflow which runs on every pull request and push.

The style checks were run only on pull request, hence ignoring the
commit messages of a branch in the pull request.

See also this issue of the checker:
mristin/opinionated-commit-message#28
mristin added a commit to admin-shell-io/aasx-package-explorer that referenced this issue Jul 11, 2020
This splits the commit message checks from the check styles into a
separate workflow which runs on every pull request and push.

The style checks were run only on pull request, hence ignoring the
commit messages of a branch in the pull request.

See also this issue of the checker:
mristin/opinionated-commit-message#28
mristin added a commit to admin-shell-io/aasx-package-explorer that referenced this issue Jul 11, 2020
This splits the commit message checks from the check styles into a
separate workflow which runs on every pull request and push.

The style checks were run only on pull request, hence ignoring the
commit messages of a branch in the pull request.

See also this issue of the checker:
mristin/opinionated-commit-message#28
@mristin
Copy link
Owner

mristin commented Jan 27, 2022

Closing since no new messages in a while.

@mristin mristin closed this as completed Jan 27, 2022
@1david5
Copy link

1david5 commented Jun 15, 2022

hey @mristin, I just noticed that when the action gets triggered by push it only checks the last commit in the PR. On the other hand. When it's triggered by a pull_request it checks the first commit in the PR.

Is there any way to trigger the action to check all the commits in the PR like the commit-message-checker does?

@mristin
Copy link
Owner

mristin commented Jun 15, 2022

Hi @1david5 ,
I'll re-visit commit-message-checker and see how they check the commits. Please have patience, though, as I am currently pretty tight on time.

@1david5
Copy link

1david5 commented Jun 15, 2022

That sounds great. Thank you for the update and this action 👍

@mristin
Copy link
Owner

mristin commented Jun 24, 2022

Hi @1david5 ,
Please apologize -- I'm swamped with the work at the moment, and probably all the way till before the summer holidays in July. Would you perhaps mind looking and debugging the issue yourself, and creating a pull request?

@mristin mristin reopened this Jun 24, 2022
@1david5
Copy link

1david5 commented Jun 24, 2022

Hi @mristin,
Yeah I understand, I'm pretty much the same but still gonna try to submit the PR with a fix since still want to use the action. Thank you 👍

@mristin
Copy link
Owner

mristin commented Sep 3, 2022

Hi @1david5 ,
Please apologize again. I am still battling with the same things as in June. I hoped that one phase of the project would have finished during summer, but here we are...

At this point I can't really promise any timeline when I'd be able to look into this. If you can scrap some time, please do go ahead and make a PR. I promise to review it & support you as much I can.

johnboyes added a commit to johnboyes/commit-checker-example that referenced this issue Sep 11, 2022
The PR body doesn't form part of the commit, so there is no need to
check it.

NB If the commit message checker changes in the future to [check the PR
commit messages rather than the PR body][1] then we can reinstate the
checks then.

[1]: mristin/opinionated-commit-message#28
johnboyes added a commit to johnboyes/commit-checker-example that referenced this issue Sep 11, 2022
The PR body doesn't form part of the commit, so there is no need to
check it.

NB If the commit message checker changes in the future to [check the PR
commit messages rather than the PR body][1] then we can reinstate the
checks then.

[1]: mristin/opinionated-commit-message#28
johnboyes added a commit to johnboyes/commit-checker-example that referenced this issue Sep 11, 2022
The PR body doesn't form part of the commit, so there is no need to
check it.

NB If the commit message checker changes in the future to [check the PR
commit messages rather than the PR body][1] then we can reinstate the
checks then.

[1]: mristin/opinionated-commit-message#28
johnboyes added a commit to johnboyes/commit-checker-example that referenced this issue Sep 11, 2022
The PR body doesn't form part of the commit, so there is no need to
check it.

NB If the commit message checker changes in the future to [check the PR
commit messages rather than the PR body][1] then we can reinstate the
checks then.

[1]: mristin/opinionated-commit-message#28
johnboyes added a commit to johnboyes/commit-checker-example that referenced this issue Sep 11, 2022
The PR body doesn't form part of the commit, so there is no need to
check it.

NB If the commit message checker changes in the future to [check the PR
commit messages rather than the PR body][1] then we can reinstate the
checks then.

[1]: mristin/opinionated-commit-message#28
johnboyes added a commit to johnboyes/commit-checker-example that referenced this issue Sep 11, 2022
The PR body doesn't form part of the commit, so there is no need to
check it.

NB If the commit message checker changes in the future to [check the PR
commit messages rather than the PR body][1] then we can reinstate the
checks then.

[1]: mristin/opinionated-commit-message#28
johnboyes added a commit to agilepathway/label-checker that referenced this issue Sep 11, 2022
The PR body doesn't form part of the commit, so there is no need to
check it.

NB If the commit message checker changes in the future to [check the PR
commit messages rather than the PR body][1] then we can reinstate the
checks then.

[1]: mristin/opinionated-commit-message#28
johnboyes added a commit to agilepathway/label-checker that referenced this issue Sep 11, 2022
The PR body doesn't form part of the commit, so there is no need to
check it.

NB If the commit message checker changes in the future to [check the PR
commit messages rather than the PR body][1] then we can reinstate the
checks then.

[1]: mristin/opinionated-commit-message#28
@mristin
Copy link
Owner

mristin commented Dec 31, 2022

@johnboyes @1david5 I finally have some time to fix this.

I thought to check both the body of pull request and the commit messages. I'd also add a flag, dont_check_commit_messages, and release a major version.

What do you think?

@johnboyes
Copy link

Hi @mristin, thanks for working on this. For the flag, I think a positive flag (rather than a negative flag starting with dont) would be more intuitive to use, called something like check_pr_commits. I think it's important to include pr in the flag name, otherwise the name would be confusing.
In terms of checking the pull request body, I'd propose a separate flag for that, called something like check_pr_body. HTH, John

@Fryuni
Copy link
Contributor

Fryuni commented Oct 31, 2023

Hi everyone! I'm working on this issue at the moment, so as an update.

The event received by the GH action doesn't include the commits of the PR, it includes the URL where those commits can be retrieved (following HATEOAS). Problem is that the commits URL has the same privacy as the repo, so for private repos this action would need the GITHUB_TOKEN secret in order to validate the commit messages like so:

- uses: mristin/opinionated-commit-message@v3.0.1
  with:
    github-token: ${ secrets.GITHUB_TOKEN }

Public repos don't have such requirement.

I'll try to send a PR soon with this.

@Fryuni
Copy link
Contributor

Fryuni commented Nov 6, 2023

PR opened, please try it out!

mristin pushed a commit that referenced this issue Nov 15, 2023
With this patch, we verify not only the latest commit message, but also
check that *all* commit messages are verified.

Fixes #28.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants