Re: [PATCH v2 8/8] check-whitespace: detect if no base_commit is provided
From: Justin Tobler <hidden>
Date: 2024-07-11 14:49:22
On 24/07/11 10:30AM, Karthik Nayak wrote:
quoted hunk ↗ jump to hunk
The 'check-whitespace' CI script exits gracefully if no base commit is provided or if an invalid revision is provided. This is not good because if a particular CI provides an incorrect base_commit, it would fail successfully. This is exactly the case with the GitLab CI. The CI is using the "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" variable to get the base commit SHA, but variable is only defined for _merged_ pipelines. So it is empty for regular pipelines [1]. This should've failed the check-whitespace job. Let's fallback to 'CI_MERGE_REQUEST_DIFF_BASE_SHA' if "CI_MERGE_REQUEST_TARGET_BRANCH_SHA" isn't available in GitLab CI, similar to the previous commit. Let's also add a check for incorrect base_commit in the 'check-whitespace.sh' script. While here, fix a small typo too. [1]: https://docs.gitlab.com/ee/ci/variables/predefined_variables.html#predefined-variables-for-merge-request-pipelines Signed-off-by: Karthik Nayak <redacted> --- .gitlab-ci.yml | 7 ++++++- ci/check-whitespace.sh | 10 ++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-)diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index dc43fc8ba8..c5a18f655a 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml@@ -119,7 +119,12 @@ check-whitespace: before_script: - ./ci/install-dependencies.sh script: - - ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" + - | + if [ -z ${CI_MERGE_REQUEST_TARGET_BRANCH_SHA} ]; then + ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_DIFF_BASE_SHA" + else + ./ci/check-whitespace.sh "$CI_MERGE_REQUEST_TARGET_BRANCH_SHA" + fi
Not worth a reroll, but it would be nice to have a comment here explaining why we have this fallback as, to me at least, it is not very obvious.
rules:
- if: $CI_PIPELINE_SOURCE == 'merge_request_event'
[snip] Overall the GitLab CI changes look good to me. Thanks :) -Justin