Re: [PATCH 7/8] ci: run style check on GitHub and GitLab

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jul 8, 2024, 21:16 UTC
Message-ID: <CAOLa=ZTuBziBPY1B0P3gTWMhqThuyFsFW1aWtwUconYN4gH+aA@mail.gmail.com>
In-Reply-To: <7wc2ucdbirqgtrxgij3i4eqwfib334kdogbfxtiyifje6clsat@3p3xnqkoj3ic>

Justin Tobler <jltobler@gmail.com> writes:

Show 33 quoted lines

> On 24/07/08 11:23AM, Karthik Nayak wrote:
>> We don't run style checks on our CI, even though we have a
>> '.clang-format' setup in the repository. Let's add one, the job will
>> validate only against the new commits added and will only run on merge
>> requests. Since we're introducing it for the first time, let's allow
>> this job to fail, so we can validate if this is useful and eventually
>> enforce it.
>
> [snip]
>
>> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
>> index 37b991e080..65fd261e5e 100644
>> --- a/.gitlab-ci.yml
>> +++ b/.gitlab-ci.yml
>> @@ -123,6 +123,18 @@ check-whitespace:
>>    rules:
>>      - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>>
>> +check-style:
>> +  image: ubuntu:latest
>> +  allow_failure: true
>> +  variables:
>> +    CC: clang
>> +  before_script:
>> +    - ./ci/install-dependencies.sh
>> +  script:
>> +    - ./ci/run-style-check.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA"
>
> One downside to using $CI_MERGE_REQUEST_DIFF_BASE_SHA is that for GitLab
> merge pipeines, commits from the merge that are not part of the MR
> changes are also included. This could lead to somewhat confusing
> failures.
>

I'm not sure I follow.

> Example failure occuring on this patch series:
> https://gitlab.com/gitlab-org/git/-/jobs/7284442220
>

If you notice this job, it points to the commit: 1c6551488, and the parent commit of that commit is: 614dff2011.

The parent commit [1] is a test commit I added to check the failures. So isn't this working as expected?

> It might be best to use $CI_MERGE_REQUEST_TARGET_BRANCH_SHA instead.
>

I actually started with $CI_MERGE_REQUEST_TARGET_BRANCH_SHA, it didn't work, because the value was undefined.

See: https://gitlab.com/gitlab-org/git/-/jobs/7283724903

This is why I also decided to fix and change the whitespace check.

>> +  rules:
>> +    - if: $CI_PIPELINE_SOURCE == 'merge_request_event'
>> +

[1]: https://gitlab.com/gitlab-org/git/-/commit/614dff20119aa325661424a9fcef552e242a95d9