lint: Add pre-merge checks using pre-push hook
diff mbox series

Message ID 20240614155718.10902-1-kieran.bingham@ideasonboard.com
State Accepted
Headers show
Series
  • lint: Add pre-merge checks using pre-push hook
Related show

Commit Message

Kieran Bingham June 14, 2024, 3:57 p.m. UTC
Wrap the existing libcamera pre-push hook into a lint task to make sure
that the rules we apply to merging are conveyed as part of the CI jobs.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 .gitlab-ci/lint-pre-push.sh | 27 +++++++++++++++++++++++++++
 gitlab-ci.yml               | 16 +++++++++++++++-
 2 files changed, 42 insertions(+), 1 deletion(-)
 create mode 100755 .gitlab-ci/lint-pre-push.sh

Comments

Kieran Bingham June 14, 2024, 4:02 p.m. UTC | #1
Quoting Kieran Bingham (2024-06-14 16:57:18)
> Wrap the existing libcamera pre-push hook into a lint task to make sure
> that the rules we apply to merging are conveyed as part of the CI jobs.
> 

A couple of test/examples:

Successful pipeline: https://gitlab.freedesktop.org/kbingham/libcamera/-/pipelines/1201492
Failed pipeline: https://gitlab.freedesktop.org/kbingham/libcamera/-/pipelines/1201497

--
Kieran

> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  .gitlab-ci/lint-pre-push.sh | 27 +++++++++++++++++++++++++++
>  gitlab-ci.yml               | 16 +++++++++++++++-
>  2 files changed, 42 insertions(+), 1 deletion(-)
>  create mode 100755 .gitlab-ci/lint-pre-push.sh
> 
> diff --git a/.gitlab-ci/lint-pre-push.sh b/.gitlab-ci/lint-pre-push.sh
> new file mode 100755
> index 000000000000..8a4283cc0f15
> --- /dev/null
> +++ b/.gitlab-ci/lint-pre-push.sh
> @@ -0,0 +1,27 @@
> +#!/bin/bash
> +
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# SPDX-FileCopyrightText: © 2023 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +#
> +# Pre-merge checks
> +
> +set -e
> +
> +source "$(dirname "$0")/lib.sh"
> +
> +libcamera_premerge() {
> +       echo "Running Pre-merge checks for $CI_COMMIT_REF_NAME ($base..$CI_COMMIT_SHA)"
> +
> +       # Wrap parameters as git would send them to the pre-push hooks directly.
> +       echo "$CI_COMMIT_REF_NAME $CI_COMMIT_SHA refs/heads/integration/pre-push-lint-test $base" \
> +               | ./utils/hooks/pre-push origin "$CI_DEFAULT_BRANCH"
> +}
> +
> +base=$(find_base origin/$CI_DEFAULT_BRANCH $CI_COMMIT_SHA)
> +
> +if [[ $base == $CI_COMMIT_SHA ]] ; then
> +       echo "No commit to test, skipping"
> +       exit 0
> +fi
> +
> +run libcamera_premerge
> diff --git a/gitlab-ci.yml b/gitlab-ci.yml
> index 50b81e591458..d9b31ec8259e 100644
> --- a/gitlab-ci.yml
> +++ b/gitlab-ci.yml
> @@ -311,7 +311,7 @@ build-package:cros:
>        dotenv: env
>  
>  # ------------------------------------------------------------------------------
> -# Lint stage - Run checkstyle.py
> +# Lint stage - Run checkstyle.py and check merge suitability
>  # ------------------------------------------------------------------------------
>  
>  lint:
> @@ -330,6 +330,20 @@ lint:
>    script:
>      - $CI_PROJECT_DIR/.gitlab-ci/lint-libcamera.sh
>  
> +merge-check:
> +  extends:
> +    - .fdo.distribution-image@debian
> +    - .history-jobs
> +    - .libcamera-ci.debian:12
> +    - .libcamera-ci.scripts
> +  stage: lint
> +  needs:
> +    - job: container-debian:12
> +      artifacts: false
> +  script:
> +    - $CI_PROJECT_DIR/.gitlab-ci/lint-pre-push.sh
> +
> +
>  # ------------------------------------------------------------------------------
>  # Test stage - Run unit tests and hardware tests
>  # ------------------------------------------------------------------------------
> -- 
> 2.34.1
>
Laurent Pinchart June 14, 2024, 6:24 p.m. UTC | #2
Hi Kieran,

Thank you for the patch.

On Fri, Jun 14, 2024 at 04:57:18PM +0100, Kieran Bingham wrote:
> Wrap the existing libcamera pre-push hook into a lint task to make sure
> that the rules we apply to merging are conveyed as part of the CI jobs.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  .gitlab-ci/lint-pre-push.sh | 27 +++++++++++++++++++++++++++
>  gitlab-ci.yml               | 16 +++++++++++++++-
>  2 files changed, 42 insertions(+), 1 deletion(-)
>  create mode 100755 .gitlab-ci/lint-pre-push.sh
> 
> diff --git a/.gitlab-ci/lint-pre-push.sh b/.gitlab-ci/lint-pre-push.sh
> new file mode 100755
> index 000000000000..8a4283cc0f15
> --- /dev/null
> +++ b/.gitlab-ci/lint-pre-push.sh
> @@ -0,0 +1,27 @@
> +#!/bin/bash
> +
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# SPDX-FileCopyrightText: © 2023 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +#
> +# Pre-merge checks
> +
> +set -e
> +
> +source "$(dirname "$0")/lib.sh"
> +
> +libcamera_premerge() {
> +	echo "Running Pre-merge checks for $CI_COMMIT_REF_NAME ($base..$CI_COMMIT_SHA)"
> +
> +	# Wrap parameters as git would send them to the pre-push hooks directly.
> +	echo "$CI_COMMIT_REF_NAME $CI_COMMIT_SHA refs/heads/integration/pre-push-lint-test $base" \
> +		| ./utils/hooks/pre-push origin "$CI_DEFAULT_BRANCH"
> +}
> +
> +base=$(find_base origin/$CI_DEFAULT_BRANCH $CI_COMMIT_SHA)
> +
> +if [[ $base == $CI_COMMIT_SHA ]] ; then
> +	echo "No commit to test, skipping"
> +	exit 0
> +fi
> +
> +run libcamera_premerge
> diff --git a/gitlab-ci.yml b/gitlab-ci.yml
> index 50b81e591458..d9b31ec8259e 100644
> --- a/gitlab-ci.yml
> +++ b/gitlab-ci.yml
> @@ -311,7 +311,7 @@ build-package:cros:
>        dotenv: env
>  
>  # ------------------------------------------------------------------------------
> -# Lint stage - Run checkstyle.py
> +# Lint stage - Run checkstyle.py and check merge suitability
>  # ------------------------------------------------------------------------------
>  
>  lint:
> @@ -330,6 +330,20 @@ lint:
>    script:
>      - $CI_PROJECT_DIR/.gitlab-ci/lint-libcamera.sh
>  
> +merge-check:
> +  extends:
> +    - .fdo.distribution-image@debian
> +    - .history-jobs
> +    - .libcamera-ci.debian:12
> +    - .libcamera-ci.scripts
> +  stage: lint
> +  needs:
> +    - job: container-debian:12
> +      artifacts: false
> +  script:
> +    - $CI_PROJECT_DIR/.gitlab-ci/lint-pre-push.sh

Making this a hard failure means that we'll have to allow merging
branches that fail CI in some cases (I'm thinking for instance of urgent
fixes to the master branch that can't get a review in time). For now
that's not a problem as CI doesn't gate merging. If that changes in the
future we'll have to either make sure we can merge with failed CI
pipelines, or update this check someone. That's an issue with can deal
with later, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +
>  # ------------------------------------------------------------------------------
>  # Test stage - Run unit tests and hardware tests
>  # ------------------------------------------------------------------------------
Kieran Bingham June 15, 2024, 12:55 p.m. UTC | #3
Quoting Laurent Pinchart (2024-06-14 19:24:20)
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Fri, Jun 14, 2024 at 04:57:18PM +0100, Kieran Bingham wrote:
> > Wrap the existing libcamera pre-push hook into a lint task to make sure
> > that the rules we apply to merging are conveyed as part of the CI jobs.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  .gitlab-ci/lint-pre-push.sh | 27 +++++++++++++++++++++++++++
> >  gitlab-ci.yml               | 16 +++++++++++++++-
> >  2 files changed, 42 insertions(+), 1 deletion(-)
> >  create mode 100755 .gitlab-ci/lint-pre-push.sh
> > 
> > diff --git a/.gitlab-ci/lint-pre-push.sh b/.gitlab-ci/lint-pre-push.sh
> > new file mode 100755
> > index 000000000000..8a4283cc0f15
> > --- /dev/null
> > +++ b/.gitlab-ci/lint-pre-push.sh
> > @@ -0,0 +1,27 @@
> > +#!/bin/bash
> > +
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +# SPDX-FileCopyrightText: © 2023 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > +#
> > +# Pre-merge checks
> > +
> > +set -e
> > +
> > +source "$(dirname "$0")/lib.sh"
> > +
> > +libcamera_premerge() {
> > +     echo "Running Pre-merge checks for $CI_COMMIT_REF_NAME ($base..$CI_COMMIT_SHA)"
> > +
> > +     # Wrap parameters as git would send them to the pre-push hooks directly.
> > +     echo "$CI_COMMIT_REF_NAME $CI_COMMIT_SHA refs/heads/integration/pre-push-lint-test $base" \
> > +             | ./utils/hooks/pre-push origin "$CI_DEFAULT_BRANCH"
> > +}
> > +
> > +base=$(find_base origin/$CI_DEFAULT_BRANCH $CI_COMMIT_SHA)
> > +
> > +if [[ $base == $CI_COMMIT_SHA ]] ; then
> > +     echo "No commit to test, skipping"
> > +     exit 0
> > +fi
> > +
> > +run libcamera_premerge
> > diff --git a/gitlab-ci.yml b/gitlab-ci.yml
> > index 50b81e591458..d9b31ec8259e 100644
> > --- a/gitlab-ci.yml
> > +++ b/gitlab-ci.yml
> > @@ -311,7 +311,7 @@ build-package:cros:
> >        dotenv: env
> >  
> >  # ------------------------------------------------------------------------------
> > -# Lint stage - Run checkstyle.py
> > +# Lint stage - Run checkstyle.py and check merge suitability
> >  # ------------------------------------------------------------------------------
> >  
> >  lint:
> > @@ -330,6 +330,20 @@ lint:
> >    script:
> >      - $CI_PROJECT_DIR/.gitlab-ci/lint-libcamera.sh
> >  
> > +merge-check:
> > +  extends:
> > +    - .fdo.distribution-image@debian
> > +    - .history-jobs
> > +    - .libcamera-ci.debian:12
> > +    - .libcamera-ci.scripts
> > +  stage: lint
> > +  needs:
> > +    - job: container-debian:12
> > +      artifacts: false
> > +  script:
> > +    - $CI_PROJECT_DIR/.gitlab-ci/lint-pre-push.sh
> 
> Making this a hard failure means that we'll have to allow merging
> branches that fail CI in some cases (I'm thinking for instance of urgent
> fixes to the master branch that can't get a review in time). For now

I think if one of us is merging a patch without a review, they can
supply an 'Acked-by: tag' to acknowledge that responsibility.

That's how I get around the merge rule on release commits which I don't
get review tags for.

I can't imagine anything really being  'so urgent' it can't get an 'Ok
sure - go ahead and merge that'. We don't have hard-realtime constraints
on merging.

> that's not a problem as CI doesn't gate merging. If that changes in the
> future we'll have to either make sure we can merge with failed CI
> pipelines, or update this check someone. That's an issue with can deal
> with later, so

Yes, I think this is something we can consider if it comes up. And at
the moment, I think the likelyhood should be low - because the whole
point of the CI is to make the likelyhood lower that we merge anything
unsuitable in the first place.


And this specific instance can be resolved with an:

Acked-by: Author <of@commit.com>, which even publicly records that the
action was taken to bypass a check.

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks.

Kieran


> 
> > +
> > +
> >  # ------------------------------------------------------------------------------
> >  # Test stage - Run unit tests and hardware tests
> >  # ------------------------------------------------------------------------------
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart June 15, 2024, 1:48 p.m. UTC | #4
On Sat, Jun 15, 2024 at 01:55:02PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-06-14 19:24:20)
> > Hi Kieran,
> > 
> > Thank you for the patch.
> > 
> > On Fri, Jun 14, 2024 at 04:57:18PM +0100, Kieran Bingham wrote:
> > > Wrap the existing libcamera pre-push hook into a lint task to make sure
> > > that the rules we apply to merging are conveyed as part of the CI jobs.
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  .gitlab-ci/lint-pre-push.sh | 27 +++++++++++++++++++++++++++
> > >  gitlab-ci.yml               | 16 +++++++++++++++-
> > >  2 files changed, 42 insertions(+), 1 deletion(-)
> > >  create mode 100755 .gitlab-ci/lint-pre-push.sh
> > > 
> > > diff --git a/.gitlab-ci/lint-pre-push.sh b/.gitlab-ci/lint-pre-push.sh
> > > new file mode 100755
> > > index 000000000000..8a4283cc0f15
> > > --- /dev/null
> > > +++ b/.gitlab-ci/lint-pre-push.sh
> > > @@ -0,0 +1,27 @@
> > > +#!/bin/bash
> > > +
> > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > +# SPDX-FileCopyrightText: © 2023 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > +#
> > > +# Pre-merge checks
> > > +
> > > +set -e
> > > +
> > > +source "$(dirname "$0")/lib.sh"
> > > +
> > > +libcamera_premerge() {
> > > +     echo "Running Pre-merge checks for $CI_COMMIT_REF_NAME ($base..$CI_COMMIT_SHA)"
> > > +
> > > +     # Wrap parameters as git would send them to the pre-push hooks directly.
> > > +     echo "$CI_COMMIT_REF_NAME $CI_COMMIT_SHA refs/heads/integration/pre-push-lint-test $base" \
> > > +             | ./utils/hooks/pre-push origin "$CI_DEFAULT_BRANCH"
> > > +}
> > > +
> > > +base=$(find_base origin/$CI_DEFAULT_BRANCH $CI_COMMIT_SHA)
> > > +
> > > +if [[ $base == $CI_COMMIT_SHA ]] ; then
> > > +     echo "No commit to test, skipping"
> > > +     exit 0
> > > +fi
> > > +
> > > +run libcamera_premerge
> > > diff --git a/gitlab-ci.yml b/gitlab-ci.yml
> > > index 50b81e591458..d9b31ec8259e 100644
> > > --- a/gitlab-ci.yml
> > > +++ b/gitlab-ci.yml
> > > @@ -311,7 +311,7 @@ build-package:cros:
> > >        dotenv: env
> > >  
> > >  # ------------------------------------------------------------------------------
> > > -# Lint stage - Run checkstyle.py
> > > +# Lint stage - Run checkstyle.py and check merge suitability
> > >  # ------------------------------------------------------------------------------
> > >  
> > >  lint:
> > > @@ -330,6 +330,20 @@ lint:
> > >    script:
> > >      - $CI_PROJECT_DIR/.gitlab-ci/lint-libcamera.sh
> > >  
> > > +merge-check:
> > > +  extends:
> > > +    - .fdo.distribution-image@debian
> > > +    - .history-jobs
> > > +    - .libcamera-ci.debian:12
> > > +    - .libcamera-ci.scripts
> > > +  stage: lint
> > > +  needs:
> > > +    - job: container-debian:12
> > > +      artifacts: false
> > > +  script:
> > > +    - $CI_PROJECT_DIR/.gitlab-ci/lint-pre-push.sh
> > 
> > Making this a hard failure means that we'll have to allow merging
> > branches that fail CI in some cases (I'm thinking for instance of urgent
> > fixes to the master branch that can't get a review in time). For now
> 
> I think if one of us is merging a patch without a review, they can
> supply an 'Acked-by: tag' to acknowledge that responsibility.
> 
> That's how I get around the merge rule on release commits which I don't
> get review tags for.
> 
> I can't imagine anything really being  'so urgent' it can't get an 'Ok
> sure - go ahead and merge that'. We don't have hard-realtime constraints
> on merging.
> 
> > that's not a problem as CI doesn't gate merging. If that changes in the
> > future we'll have to either make sure we can merge with failed CI
> > pipelines, or update this check someone. That's an issue with can deal
> > with later, so
> 
> Yes, I think this is something we can consider if it comes up. And at
> the moment, I think the likelyhood should be low - because the whole
> point of the CI is to make the likelyhood lower that we merge anything
> unsuitable in the first place.
> 
> 
> And this specific instance can be resolved with an:
> 
> Acked-by: Author <of@commit.com>, which even publicly records that the
> action was taken to bypass a check.

I like that idea.

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > > +
> > > +
> > >  # ------------------------------------------------------------------------------
> > >  # Test stage - Run unit tests and hardware tests
> > >  # ------------------------------------------------------------------------------

Patch
diff mbox series

diff --git a/.gitlab-ci/lint-pre-push.sh b/.gitlab-ci/lint-pre-push.sh
new file mode 100755
index 000000000000..8a4283cc0f15
--- /dev/null
+++ b/.gitlab-ci/lint-pre-push.sh
@@ -0,0 +1,27 @@ 
+#!/bin/bash
+
+# SPDX-License-Identifier: GPL-2.0-or-later
+# SPDX-FileCopyrightText: © 2023 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+#
+# Pre-merge checks
+
+set -e
+
+source "$(dirname "$0")/lib.sh"
+
+libcamera_premerge() {
+	echo "Running Pre-merge checks for $CI_COMMIT_REF_NAME ($base..$CI_COMMIT_SHA)"
+
+	# Wrap parameters as git would send them to the pre-push hooks directly.
+	echo "$CI_COMMIT_REF_NAME $CI_COMMIT_SHA refs/heads/integration/pre-push-lint-test $base" \
+		| ./utils/hooks/pre-push origin "$CI_DEFAULT_BRANCH"
+}
+
+base=$(find_base origin/$CI_DEFAULT_BRANCH $CI_COMMIT_SHA)
+
+if [[ $base == $CI_COMMIT_SHA ]] ; then
+	echo "No commit to test, skipping"
+	exit 0
+fi
+
+run libcamera_premerge
diff --git a/gitlab-ci.yml b/gitlab-ci.yml
index 50b81e591458..d9b31ec8259e 100644
--- a/gitlab-ci.yml
+++ b/gitlab-ci.yml
@@ -311,7 +311,7 @@  build-package:cros:
       dotenv: env
 
 # ------------------------------------------------------------------------------
-# Lint stage - Run checkstyle.py
+# Lint stage - Run checkstyle.py and check merge suitability
 # ------------------------------------------------------------------------------
 
 lint:
@@ -330,6 +330,20 @@  lint:
   script:
     - $CI_PROJECT_DIR/.gitlab-ci/lint-libcamera.sh
 
+merge-check:
+  extends:
+    - .fdo.distribution-image@debian
+    - .history-jobs
+    - .libcamera-ci.debian:12
+    - .libcamera-ci.scripts
+  stage: lint
+  needs:
+    - job: container-debian:12
+      artifacts: false
+  script:
+    - $CI_PROJECT_DIR/.gitlab-ci/lint-pre-push.sh
+
+
 # ------------------------------------------------------------------------------
 # Test stage - Run unit tests and hardware tests
 # ------------------------------------------------------------------------------