From d3e0ca18c27b3daf0aae84cc577fc02d9b1a1700 Mon Sep 17 00:00:00 2001
From: Jameson Graef Rollins <jrollins@finestructure.net>
Date: Mon, 20 Apr 2020 14:21:38 -0700
Subject: [PATCH] CI: add approval validation check for noise changing merge
requests
This adds a CI special job that runs only on merge requests that update
the test git ref, indicating that a noise has been changed. The MR will
show present a report of the changed noises, and will block on MR approval.
Once approval has been given, the check_approval job can be re-run, which
should now pass, allow the MR to proceed.
If the noises change but the reference has not been updated, then the CI
pipeline will fail as usual.
---
.gitlab-ci.yml | 74 ++++++++++++++++++++++++++++++++++++++++++-------
CONTRIBUTING.md | 21 +++++++++++++-
2 files changed, 84 insertions(+), 11 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 70d273d0..d79541d8 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -1,11 +1,20 @@
stages:
- dist
- test
- - gen_cache
- - update_cache
+ - review
- docs
- deploy
+# have to specify this so that all jobs execute for all commits
+# including merge requests
+workflow:
+ rules:
+ - if: $CI_MERGE_REQUEST_ID
+ - if: $CI_COMMIT_BRANCH
+
+variables:
+ GIT_STRATEGY: clone
+
# build the docker image we will use in all the jobs, with all
# dependencies pre-installed/configured.
gwinc/base:
@@ -19,14 +28,16 @@ gwinc/base:
cat <<EOF > Dockerfile
FROM igwn/base:buster
RUN apt-get update -qq
- RUN apt-get -y install --no-install-recommends git python3 python3-yaml python3-scipy python3-matplotlib python3-ipython lalsimulation-python3 python3-pypdf2 python3-h5py
+ RUN apt-get -y install --no-install-recommends git gitlab-cli python3 python3-yaml python3-scipy python3-matplotlib python3-ipython lalsimulation-python3 python3-pypdf2 python3-h5py
RUN git clone https://gitlab-ci-token:ci_token@git.ligo.org/gwinc/inspiral_range.git
EOF
- docker build -t $IMAGE_TAG .
- docker push $IMAGE_TAG
-# run the tests and generate the test report on failure
-test:
+# validate that the noises haven't changed relative to the reference
+# (the reference itself could have been updated, though, see
+# check_approval job)
+validate_noise:
stage: test
image: $CI_REGISTRY_IMAGE/gwinc/base:$CI_COMMIT_REF_NAME
script:
@@ -37,13 +48,58 @@ test:
when: on_failure
paths:
- gwinc_test_report.pdf
- expose_as: 'GWINC test failure report PDF'
+ expose_as: 'noise validation failure report'
+
+# this is a special job intended to run only for merge requests where
+# the test reference hash file has been updated, indicating that there
+# has been a noise change. if the merge request has not yet been
+# approved, generate a report of noise changes relative to the target
+# branch and present that to the reviewers. if the merge request is
+# approved, re-run this job, which will succeed if the MR is approved.
+check_approval:
+ stage: review
+ rules:
+ # - if: '$CI_MERGE_REQUEST_TARGET_BRANCH_NAME == "master"'
+ - if: $CI_MERGE_REQUEST_ID
+ changes:
+ - gwinc/test/ref_hash
+ image: $CI_REGISTRY_IMAGE/gwinc/base:$CI_COMMIT_REF_NAME
+ script:
+ - echo "NOISE REFERENCE CHANGE, checking approval..."
+ - |
+ cat <<EOF > check_approved.py
+ import sys
+ import gitlab
+ project_id = sys.argv[1]
+ mr_iid = sys.argv[2]
+ # this only works for public repos, otherwise need to specify
+ # private_token=
+ gl = gitlab.Gitlab('https://git.ligo.org')
+ project = gl.projects.get(project_id)
+ mr = project.mergerequests.get(mr_iid)
+ approvals = mr.approvals.get()
+ assert approvals.approved
+ EOF
+ - if ! python3 check_approved.py $CI_PROJECT_ID $CI_MERGE_REQUEST_IID ; then
+ - old_hash=$(git cat-file -p origin/$CI_MERGE_REQUEST_TARGET_BRANCH_NAME:gwinc/test/ref_hash)
+ - if ! python3 -m gwinc.test --git-ref $old_hash -r gwinc_test_report.pdf ; then
+ - echo "APPROVAL REQUIRED TO MERGE THIS BRANCH."
+ - /bin/false
+ - else
+ - echo "Reference update did not cause appreciable noise change."
+ - fi
+ - else
+ - echo "Merge request approved, reference change accepted."
+ - fi
+ artifacts:
+ when: on_failure
+ paths:
+ - gwinc_test_report.pdf
+ expose_as: 'noise changes relative to target branch head APPROVAL REQUIRED TO MERGE'
# create plots for the canonical IFOs
ifo:
stage: docs
- needs:
- - test
image: $CI_REGISTRY_IMAGE/gwinc/base:$CI_COMMIT_REF_NAME
script:
- mkdir -p ifo
@@ -63,8 +119,6 @@ html:
stage: docs
only:
- master
- needs:
- - test
image: $CI_REGISTRY_IMAGE/gwinc/base:$CI_COMMIT_REF_NAME
script:
- rm -rf public
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index d7f1edd4..c34f1d23 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -44,4 +44,23 @@ commit after the commits that change the noise calculations.*
Once you submit your merge request, an automated pipeline will run the
test script to validate your changes. If the tests fail but noise
-changes are detected, then the merge request will be rejected.
+changes are detected, then the merge request will be rejected. If
+your merge request contains changes to the test reference an alert
+will be generated in the merge request indicating that approval by
+reviewers is required before merge.
+
+
+## For admins: approving noise curve changes
+
+As discussed above, merge requests that generate noise changes will
+cause pipeline failures. If the MR properly includes a reference
+update, then the failure will only be in waiting for proper approval
+of the MR. A report will also be generated to compare all the noise
+changes against the target branch (usually 'master'). See the 'View
+exposed artifacts' menu item in the pipeline report. Once you have
+reviewed the report and the code, and understand and accept the noise
+changes, click the 'Approve' button in the MR. Once sufficient
+approval has been given, re-run the `review:check_approval` job, which
+should now pick up that approval has been given, and if everything
+goes ok the pipeline will now succeed, allowing the code to be merged.
+Click the 'Merge' button to finally merge the code.
--
GitLab