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

github: add new checklist workflow #1570

Closed
wants to merge 2 commits into from
Closed

Conversation

VexedUXR
Copy link
Contributor

Add a new 'checklist' workflow that checks the commit messages on pull
requests. Currently, the workflow creates a comment on the pull request
if any of these conditions are hit:

  • Missing Signed-off-by
  • Malformed Signed-off-by
  • Bad email (i.e *noreply*)

A test can be found here: VexedUXR#6
The comment is edited as the pull request is updated.
The comment get updated to:

Thank you for taking the time to contribute to FreeBSD!
All issues resolved.

When all issues are resolved.
If no issues are found in the first place, then no comment is added.

@VexedUXR VexedUXR requested a review from bsdimp as a code owner January 12, 2025 06:24
Copy link
Member

@bsdimp bsdimp left a comment

Choose a reason for hiding this comment

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

I think I really like these changes. Thanks! Sorry it took me a bit to get around to looking at them.

@emaste
Copy link
Member

emaste commented Jan 22, 2025

We should check that the (or, a) Signed-off-by matches the author's email too, no?

But indeed I like this. There are existing actions we could use too (e.g. https://github.com/dcoapp/app) but it's simple enough that a small bespoke GH action seems reasonable (and it's nice to capture all of the various things we want to include, like noreply emails, in one action)

jobs:
checklist:
name: commit
runs-on: ubuntu-latest
Copy link
Member

Choose a reason for hiding this comment

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

# XXX would prefer to run this on FreeBSD-latest :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, hopefully one day

@brooksdavis
Copy link
Contributor

We should check that the (or, a) Signed-off-by matches the author's email too, no?

Not necessarily, but it's probably fine to warn if they aren't the same. For example, for FreeBSD committers it wouldn't be unreasonable to have a Signed-off-by with your work email while Author is @freebsd.org.

@bsdimp
Copy link
Member

bsdimp commented Jan 22, 2025

We should check that the (or, a) Signed-off-by matches the author's email too, no?

Not necessarily, but it's probably fine to warn if they aren't the same. For example, for FreeBSD committers it wouldn't be unreasonable to have a Signed-off-by with your work email while Author is @freebsd.org.

Other projects warn when they aren't the same. Maybe we could steal their code, but convert it to a warning.
Plus Signed-off-by can be generated by multiple people in the chain between production of the code and its contribution, so we should take that into account. It's not a unique field.

@VexedUXR
Copy link
Contributor Author

Do you mean output a warning like the style checker does using https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#setting-a-warning-message?
Or should it be appended to the comment, e.g:

There are a few issues that need to be fixed:
...
Additionally, there are a few things that you might want to change:
...

The former seems to have limited visibility, there is no indication the job outputted any warnings unless you click on it.

Plus Signed-off-by can be generated by multiple people in the chain between production of the code and its contribution, so we should take that into account. It's not a unique field.

The current code already loops over all the sob lines :)

Add a new 'checklist' workflow that checks the commit messages on pull
requests. Currently, the workflow creates a comment on the pull request
if any of these conditions are hit:
- Missing Signed-off-by
- Malformed Signed-off-by
- Bad email (i.e *noreply*)

Signed-off-by: Ahmad Khalifa <ahmadkhalifa570@gmail.com>
Both the bad email check and the signoff check are handled in the
"checklist" github workflow now.

Signed-off-by: Ahmad Khalifa <ahmadkhalifa570@gmail.com>
@VexedUXR
Copy link
Contributor Author

- Print a warning if author's email isn't included in the sob lines
- Resolved merge conflicts
- Made the "freebsd.org" check case-insensitive to match behavior of checkstyle9.pl
- Also made the noreply check case-insensitive

I opted for logging the warning instead of appending it to the comment.
The test run can be found here: https://github.com/VexedUXR/freebsd-src/actions/runs/12949254004/job/36119600320?pr=6

@emaste emaste closed this Jan 24, 2025
@emaste emaste added the merged label Jan 24, 2025
freebsd-git pushed a commit that referenced this pull request Jan 24, 2025
Add a new 'checklist' workflow that checks the commit messages on pull
requests. Currently, the workflow creates a comment on the pull request
if any of these conditions are hit:
  - Missing Signed-off-by
  - Malformed Signed-off-by
  - Bad email (i.e *noreply*)

Reviewed by:	emaste, imp
Pull request:	#1570

Signed-off-by: Ahmad Khalifa <ahmadkhalifa570@gmail.com>
freebsd-git pushed a commit that referenced this pull request Jan 24, 2025
Both the bad email check and the signoff check are handled in the
"checklist" github workflow now.

Reviewed by:	emaste, imp
Pull request:	#1570

Signed-off-by: Ahmad Khalifa <ahmadkhalifa570@gmail.com>
@VexedUXR
Copy link
Contributor Author

Thanks!

emaste added a commit to emaste/freebsd that referenced this pull request Jan 25, 2025
commit 298973d9eab33705133b6cd8081de88de8a89284
Author: Ed Maste <emaste@FreeBSD.org>
Date:   Fri Jan 24 21:48:18 2025 -0500

    openssh: regen ssh_namespace.h

commit 3107488b7a1f5606dbd3f725bfe67b467dd83755
Author: Ed Maste <emaste@FreeBSD.org>
Date:   Fri Jan 24 21:45:42 2025 -0500

    openssh: run freebsd-configure.sh

commit 90fe2b07724036b43b0e8978ef892b3a07c7fabc
Author: Ed Maste <emaste@FreeBSD.org>
Date:   Fri Jan 24 16:25:59 2025 -0500

    openssh: Bump VersionAddendum date

commit 2dcb19e247b7caf46624ea234bd3576b63f3a0d1
Author: Ed Maste <emaste@FreeBSD.org>
Date:   Fri Jan 24 16:15:22 2025 -0500

    openssh: Remove rebase leftovers

commit 61d26e562c5d4adf29dcfce93e59cafbd1e7172b
Author: Ed Maste <emaste@FreeBSD.org>
Date:   Fri Jan 24 16:13:46 2025 -0500

    openssh: Break style(9) to match upstream

commit 7cf86867661ed675fad6b445f6bc91817b123b40
Merge: ca36371 126e0f4
Author: Ed Maste <emaste@FreeBSD.org>
Date:   Fri Jan 24 16:10:09 2025 -0500

    Merge commit '126e0f4f7dcb4ed8d46dc373a4d00e022c457db4' into openssh-9.9p1-update

commit ca36371
Author: Ahmad Khalifa <ahmadkhalifa570@gmail.com>
Date:   Mon Jan 6 03:52:06 2025 +0200

    checkstyle9.pl: Remove unneeded checks

    Both the bad email check and the signoff check are handled in the
    "checklist" github workflow now.

    Reviewed by:	emaste, imp
    Pull request:	freebsd#1570

    Signed-off-by: Ahmad Khalifa <ahmadkhalifa570@gmail.com>

commit a9a6a51
Author: Ahmad Khalifa <ahmadkhalifa570@gmail.com>
Date:   Sun Jan 5 07:01:07 2025 +0200

    github: Add new checklist workflow

    Add a new 'checklist' workflow that checks the commit messages on pull
    requests. Currently, the workflow creates a comment on the pull request
    if any of these conditions are hit:
      - Missing Signed-off-by
      - Malformed Signed-off-by
      - Bad email (i.e *noreply*)

    Reviewed by:	emaste, imp
    Pull request:	freebsd#1570

    Signed-off-by: Ahmad Khalifa <ahmadkhalifa570@gmail.com>

commit c814172
Author: CismonX <admin@cismon.net>
Date:   Fri Jan 3 18:52:34 2025 +0800

    open.2: update description for O_PATH

    - Add fstatfs(), fchdir(), fchroot(), extattr_*_fd(), cap_*_get(),
      cap_*_limit() to the list of syscalls that can take an O_PATH fd.
    - Remove readlinkat() from the list, since it is already discussed
      in the first few lines of the paragraph.  It was originally added
      to the list when readlinkat() adds support for non-dir fd with
      an empty relative path (as if with AT_EMPTY_PATH), however,
      such use case is also discussed in the next paragraph.
    - Add funlinkat() to the list, since it accepts an extra fd
      (of the file to be unlinked), which is worth extra mentioning.
    - Fix a syntax issue which causes a bogus space to be rendered
      before a closing parentheses.

    Signed-off-by: CismonX <admin@cismon.net>

    Reviewed by:	markj, jhb
    MFC after:	2 weeks
    Pull Request:	freebsd#1564

commit 813f244
Author: Chattrapat Sangmanee <aomsin27@hotmail.co.th>
Date:   Wed Oct 16 21:49:22 2024 +0700

    ps3disk.c: Rewrite ps3disk_transfer

    This function is bugged since the beginning, but it never hit because
    its variable doesn't allow.  However, since commit
    a77e1f0 it happen now.

    First, it assume that ds_len will always equal to real user requested
    size.  So it being used for sector count calculation.  This is no longer
    true, and will fail if attempt to read last few sectors.  Use
    bp->bio_length instead.

    Second, this being a loop is pointless because nsegs will never be > 1
    as specified at bus_dma_tag_create() call.  And all it doing is to
    repeat very same command again but with different ds_addr.  Since
    bio_driver2 tag ident pointer are being reused, the result will be
    discarded at ps3disk_intr().

    Signed-off-by: Chattrapat Sangmanee <aomsin27@hotmail.co.th>
    Reviewed by: imp,mav
    Pull Request: freebsd#1414

commit 3c61bbe
Author: Keve Müller <kevemueller@users.noreply.github.com>
Date:   Sun Oct 27 14:09:24 2024 +0100

    Update rc.initdiskless, fix error handling of remount_optional

    chkerr() ignores the exit code of a preceding mount command in case a
    file ```remount_optional``` exists.  The check is performed and a
    subshell is launched to log the informational message and return.  The
    return is executed in the context of the subshell, not the context of
    the chkerr() function, hence is a NOP.  The remount_optional check is
    hence ineffective.

    Change the code to if/then/fi, so the return is evaluated in the context
    of the chkerr function, to make the check effective.

    Reviewed by: imp, emaste
    Pull Request: freebsd#1497

commit d726bc2
Author: Franco Fichtner <franco@opnsense.org>
Date:   Thu Jan 23 12:27:09 2025 +0100

    bsdinstall: hook up help line and prompt for ZFS disk selection

    Previously we were passing the wrong variable names for the prompt and
    help line, so the intended action wasn't clear to the user.

    Reviewed by:	jhb, markj
    MFC after:	3 days
    See also:	opnsense/installer#22
    Pull Request:	freebsd#1579

commit dc27305
Author: FUKAUMI Naoki <naoki@radxa.com>
Date:   Thu Jan 23 10:43:59 2025 +0900

    ure(4): Add support for ELECOM EDC-QUA3C

    ELECOM EDC-QUA3C is a USB3.1 Gen1 Type-A/C 2.5GBASE-T network adapter.
    This also works as a cdce(4) device by:

    usbconfig -d X.Y set_config 1
      or
    usbconfig -d X.Y set_config 2

    Signed-off-by: FUKAUMI Naoki <naoki@radxa.com>

    MFC after:	2 weeks
    Pull Request:	freebsd#1578

commit 126e0f4
Author: Ed Maste <emaste@FreeBSD.org>
Date:   Fri Sep 20 05:24:23 2024 -0400

    Vendor import of OpenSSH 9.9p1

    Sponsored by:	The FreeBSD Foundation

commit d565364
Author: Ed Maste <emaste@FreeBSD.org>
Date:   Mon Jul 1 10:01:36 2024 -0400

    Vendor import of OpenSSH 9.8p1
@VexedUXR VexedUXR deleted the Checklist branch January 25, 2025 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants