Justin Tobler <jltobler@xxxxxxxxx> writes: > On 24/07/08 02:16PM, Karthik Nayak wrote: >> Justin Tobler <jltobler@xxxxxxxxx> writes: >> >> > 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? > > Ah ok, I misunderstood the setup of that CI job, but the problem is > still present. Here is an example CI job I've run demonstrating it: > > CI - https://gitlab.com/gitlab-org/git/-/jobs/7291829941 > MR - https://gitlab.com/gitlab-org/git/-/merge_requests/174 > > For the MR that spawned this CI job, This patch series is the source > branch and the target branch is a version of master one commit ahead > containing a clang format error. Because this is a merge pipeline, using > $CI_MERGE_REQUEST_DIFF_BASE_SHA will include changes from either side of > the base commit. This means it would be possible for the CI job to fail > due to commits ahead in the target branch, but not in the source branch. > For the check-whitespace CI job, I specifically chose > $CI_MERGE_REQUEST_TARGET_BRANCH_SHA for this reason.j > You're right indeed. I did some more reading about this and I think the solution lies somewhere in between.. >> >> > 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. > > I'm not seeing $CI_MERGE_REQUEST_TARGET_BRANCH_SHA as undefined in the > job. Here is a modified version on the check-style CI job printing the > environment variables: > You can see the output $ ./ci/run-style-check.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" fatal: ambiguous argument '': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git <command> [<revision>...] -- [<file>...]' This only happens if "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is empty. > https://gitlab.com/gitlab-org/git/-/jobs/7291792329#L2470 > > Do you have an example of the check-whitespace job failing in GitLab CI? > Maybe I'm missing something, but I don't see a problem. > > -Justin So I think I get the issue, GitLab has two kinds of pipelines it runs: 1. merge pipeline: Here the pipeline runs on the source branch (the feature branch which has to be merged). 2. merged pipeline: Here the pipeline creates a merge commit using the source and target branch and then runs the pipeline on the merged commit. And "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is only defined in the 'merged pipeline'. If you see the pipelines for my branch on GitLab [1]. You'll see only one of them is marked as 'merge results' and the others being marked as 'merged results'. The former includes the job I mentioned above, where "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" is not defined. I'm still not sure why it marked only one of the pipelines as such, but this means there is chance that it could happen. So I guess the best outcome is to use "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA", but fallback to "$CI_MERGE_REQUEST_DIFF_BASE_SHA", if the former is not defined. [1]: https://gitlab.com/gitlab-org/git/-/merge_requests/172/pipelines
Attachment:
signature.asc
Description: PGP signature
