On 24/07/11 10:30AM, Karthik Nayak wrote: > 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 <karthik.188@xxxxxxxxx> > --- > .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
