Re: [edk2-devel] [Patch 1/1] BaseTools/Scripts/PatchCheck: Error if commit modifies multiple packages

["Ard Biesheuvel" <ardb@kernel.org>]

On Wed, 14 Feb 2024 at 18:16, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> Hi Ard and Leif,
>
> Today in CI we do not have fine grain control to ignore specific
> CI check failures when merging.
>

I have been asking for this for a while now.

> CI checks can either be informative only in the log or blocking.
>
> For cases where a commit really should be broken up into multiple
> commits, the informative approach in a log can be easily missed
> by authors/reviewers/maintainers.
>
> For the suggestion to override a CI check, we would need a way
> to re-run CI with input from a maintainer to relax specific CI
> check(s).  The default could be all checks enabled. Some checks
> can be enabled/disabled at the package scope through ci.yaml
> file settings. This specific check is for the contents of one
> of more commits under review, so the ci.yaml at package scope
> does not apply.
>

I turned off some CI checks in packages entirely, simply because I
cannot override a single CI fail from the dashboard.

> A couple ideas
>
> 1) Make this initial version of this check informative only and
>    figure out how to make the results more visible to the
>    author/reviewers/maintainers.

Making this informative only for all PRs likely defeats the purpose.

> 2) Investigate a mechanism for a maintainer to disable a specific
>    check and re-run CI with that check disabled.
>    a) Perhaps a flag in the commit message
>    b) Perhaps a label in the PR
>

I'd lean towards a), as it will be logged in the commit history. It
also requires some extra work to respin the PR, and this should make
abuse of this feature less likely in cases where the change can easily
just be split up instead.

> It would also be helpful if a few examples from the edk2 commit
> history where this proposed CI check extension would report a
> failure and it would not be possible to reorganize the commits into
> a passing condition. That would help support the hard requirement
> for the need to bypass the check.
>

I didn't look at all the patches in the ticket, but there are at least
two which cannot be split up without breaking the build.

103fa647d159e3d76be2634d2653c2d215dd0d46
ArmPkg: Replace CoreId and ClusterId with Mpidr in ARM_CORE_INFO struct

4c55f6394fafe0494ec24e7c05cb68c938d7852d
MdePkg: IORT header update for IORT Rev E.d spec


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115470): https://edk2.groups.io/g/devel/message/115470
Mute This Topic: https://groups.io/mt/104345509/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-