diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 70d273d043b34520b7835a94b3cc953b0f17fefc..d79541d8d44de1d4908f89c8d68101a459e7d9bd 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 d7f1edd44416e181f0f3e9ff9f3a9dec14e2a87b..c34f1d231670a5ded7f620df09ac00e160ee6a08 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.