ci: Safe commenting on PRs (#1729)

* ci: Safe commenting on PRs This generalizes and refactors what the QNS workflow did. * Test * Add missing file * Finalize * Fixes * While I'm here, make `actionlint` happy * Fix * Can't quote BUILD_TYPE * Can't quote CONFIG_PATH * Condition is already in called action * Proper quoting for backslashes * Windows again * Finalize
mozilla · Mar 11, 2024 · 532dcc5 · 532dcc5
1 parent b4fd413
commit 532dcc5
Show file tree

Hide file tree

Showing 7 changed files with 142 additions and 83 deletions.
diff --git a/.github/actions/pr-comment-data-export/action.yml b/.github/actions/pr-comment-data-export/action.yml
@@ -0,0 +1,37 @@
+name: 'Export data for PR comment'
+description: 'Exports the neccessary data to post a PR comment securely.'
+
+# This action might be running off of a fork and would thus not have write
+# permissions on the origin repository. In order to allow a separate
+# priviledged action to post a comment on a pull request, upload the
+# necessary metadata.
+
+inputs:
+  name:
+    description: 'A unique name for the artifact used for exporting.'
+    required: true
+  contents:
+    description: 'A filename with a comment (in Markdown) to be added to the PR.'
+    required: true
+  log-url:
+    description: 'A URL to a log to be linked from the PR comment.'
+    required: false
+
+runs:
+  using: composite
+  steps:
+    - if: github.event_name == 'pull_request'
+      shell: bash
+      run: |
+        mkdir comment-data
+        cp "${{ inputs.contents }}" comment-data/contents
+        echo "${{ inputs.name }}" > comment-data/name
+        echo "${{ inputs.log-url }}" > comment-data/log-url
+        echo "${{ github.event.number }}" > comment-data/pr-number
+
+    - if: github.event_name == 'pull_request'
+      uses: actions/upload-artifact@v4
+      with:
+        name: ${{ inputs.name }}
+        path: comment-data
+        retention-days: 1
diff --git a/.github/actions/pr-comment/action.yml b/.github/actions/pr-comment/action.yml
@@ -0,0 +1,29 @@
+name: 'Comment on PR'
+description: 'Post a PR comment securely.'
+
+inputs:
+  name:
+    description: 'Artifact name to import comment data from.'
+    required: true
+
+runs:
+  using: composite
+  steps:
+    - uses: actions/download-artifact@v4
+      with:
+        run-id: ${{ github.event.workflow_run.id }}
+        name: ${{ inputs.name }}
+
+    - id: pr-number
+      shell: bash
+      run: echo "number=$(cat pr-number)" >> "$GITHUB_OUTPUT"
+
+    - shell: bash
+      run: |
+        [ -s log-url ] && echo "" >> contents && echo "[:arrow_down: Download logs]($(cat log-url))" >> contents
+
+    - uses: thollander/actions-comment-pull-request@v2
+      with:
+        filePath: contents
+        pr_number: ${{ steps.pr-number.outputs.number }}
+        comment_tag: ${{ inputs.name }}-comment
diff --git a/.github/actions/quic-interop-runner/action.yml b/.github/actions/quic-interop-runner/action.yml
@@ -88,24 +88,19 @@ runs:
         name: logs
         path: quic-interop-runner/logs
 
-    # This action might be running off of a fork and would thus not have write
-    # permissions on the origin repository. In order to allow a separate
-    # priviledged action to post a comment on a pull request, upload the
-    # necessary metadata.
-    - name: store comment-data
-      shell: bash
-      if: github.event_name == 'pull_request'
-      env:
-        PULL_REQUEST_NUMBER: ${{ github.event.number }}
+    - name: Format GitHub comment
       run: |
-        mkdir comment-data
-        mv quic-interop-runner/summary comment-data/summary
-        echo $PULL_REQUEST_NUMBER > comment-data/pr-number
-        echo '${{ steps.artifact-upload-step.outputs.artifact-url }}' > comment-data/logs-url
+        echo '[**QUIC Interop Runner**](https://github.com/quic-interop/quic-interop-runner)' >> comment
+        echo '' >> comment
+        echo '```' >> comment
+        cat quic-interop-runner/summary >> comment
+        echo '```' >> comment
+        echo '' >> comment
+      shell: bash
 
-    - name: Upload comment data
-      uses: actions/upload-artifact@v4
-      if: github.event_name == 'pull_request'
+    - name: Export PR comment data
+      uses: ./.github/actions/pr-comment-data-export
       with:
-        name: comment-data
-        path: ./comment-data
+        name: qns
+        contents: comment
+        log-url: ${{ steps.artifact-upload-step.outputs.artifact-url }}
diff --git a/.github/workflows/bench-comment.yml b/.github/workflows/bench-comment.yml
@@ -0,0 +1,24 @@
+# Post test results as pull request comment.
+#
+# This is done as a separate workflow as it requires write permissions. The
+# tests itself might run off of a fork, i.e., an untrusted environment and should
+# thus not be granted write permissions.
+
+name: Benchmark Comment
+
+on:
+  workflow_run:
+    workflows: ["Bench"]
+    types:
+      - completed
+
+jobs:
+  comment:
+    permissions:
+      pull-requests: write
+    runs-on: ubuntu-latest
+    if: github.event.workflow_run.event == 'pull_request'
+    steps:
+      - uses: ./.github/actions/pr-comment
+        with:
+          name: bench
diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml
@@ -1,14 +1,17 @@
 name: Bench
 on:
-  workflow_call:
+  workflow_run:
+    workflows: ["CI"]
+    types:
+      - completed
   workflow_dispatch:
 env:
   CARGO_PROFILE_BENCH_BUILD_OVERRIDE_DEBUG: true
   CARGO_PROFILE_RELEASE_DEBUG: true
   CARGO_TERM_COLOR: always
   RUST_BACKTRACE: 1
   TOOLCHAIN: nightly
-  RUSTFLAGS: -C link-arg=-fuse-ld=lld -C link-arg=-Wl,--no-rosegment, -Cforce-frame-pointers=yes
+  RUSTFLAGS: -C link-arg=-fuse-ld=lld -C link-arg=-Wl,--no-rosegment, -C force-frame-pointers=yes
   PERF_CMD: record -o perf.data -F997 --call-graph fp -g
 
 jobs:
@@ -39,8 +42,8 @@ jobs:
 
       - name: Build
         run: |
-          cargo +$TOOLCHAIN bench --features bench --no-run
-          cargo +$TOOLCHAIN build --release --bin neqo-client --bin neqo-server
+          cargo "+$TOOLCHAIN" bench --features bench --no-run
+          cargo "+$TOOLCHAIN" build --release --bin neqo-client --bin neqo-server
 
       - name: Download cached main-branch results
         id: criterion-cache
@@ -58,7 +61,7 @@ jobs:
       - name: Run cargo bench
         run: |
           taskset -c 0 nice -n -20 \
-            cargo +$TOOLCHAIN bench --features bench | tee results.txt
+            cargo "+$TOOLCHAIN" bench --features bench | tee results.txt
 
       # Pin the transfer benchmark to core 0 and run it at elevated priority inside perf.
       # Work around https://github.com/flamegraph-rs/flamegraph/issues/248 by passing explicit perf arguments.
@@ -69,7 +72,7 @@ jobs:
           mv target/criterion target/criterion-bench
           mv target/criterion-transfer-profile target/criterion || true
           taskset -c 0 nice -n -20 \
-            cargo +$TOOLCHAIN flamegraph -v -c "$PERF_CMD" --features bench --bench transfer -- \
+            cargo "+$TOOLCHAIN" flamegraph -v -c "$PERF_CMD" --features bench --bench transfer -- \
               --bench --exact "Run multiple transfers with varying seeds"
           # And now restore the directories.
           mv target/criterion target/criterion-transfer-profile
@@ -80,13 +83,13 @@ jobs:
           { mkdir server; \
             cd server; \
             taskset -c 0 nice -n -20 \
-            cargo +$TOOLCHAIN flamegraph -v -c "$PERF_CMD" \
-              --bin neqo-server -- --db ../test-fixture/db $HOST:4433 || true; } &
+            cargo "+$TOOLCHAIN" flamegraph -v -c "$PERF_CMD" \
+              --bin neqo-server -- --db ../test-fixture/db "$HOST:4433" || true; } &
           mkdir client; \
             cd client; \
             time taskset -c 1 nice -n -20 \
-            cargo +$TOOLCHAIN flamegraph -v -c "$PERF_CMD" \
-              --bin neqo-client -- --output-dir . https://$HOST:4433/$SIZE
+            cargo "+$TOOLCHAIN" flamegraph -v -c "$PERF_CMD" \
+              --bin neqo-client -- --output-dir . "https://$HOST:4433/$SIZE"
           killall -INT neqo-server
           cd ${{ github.workspace }}
           [ "$(wc -c < client/"$SIZE")" -eq "$SIZE" ] || exit 1
@@ -148,12 +151,10 @@ jobs:
                     -e 's/(change:[^%]*%)([^%]*%)(.*)/\1**\2**\3/gi' \
               > results.md
           echo '' >> results.md
-          echo '[:arrow_down: Download full results](${{ steps.export.outputs.artifact-url }})' >> results.md
 
-      - name: "Post results to PR"
-        uses: thollander/actions-comment-pull-request@v2
+      - name: Export PR comment data
+        uses: ./.github/actions/pr-comment-data-export
         with:
-          filePath: results.md
-          pr_number: ${{ github.event.pull_request.number }}
-          comment_tag: bench-results
-
+          name: bench
+          contents: results.md
+          log-url: ${{ steps.export.outputs.artifact-url }}
diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml
@@ -72,8 +72,11 @@ jobs:
       - name: Use MSYS2 environment and install more dependencies (Windows)
         if: runner.os == 'Windows'
         run: |
-          echo "C:\\msys64\\usr\\bin" >> "$GITHUB_PATH"
-          echo "C:\\msys64\\mingw64\\bin" >> "$GITHUB_PATH"
+          # shellcheck disable=SC2028
+          {
+            echo "C:\\msys64\\usr\\bin"
+            echo "C:\\msys64\\mingw64\\bin"
+          } >> "$GITHUB_PATH"
           /c/msys64/usr/bin/pacman -S --noconfirm nsinstall
           python3 -m pip install git+https://github.com/nodejs/gyp-next
           echo "$(python3 -m site --user-base)/bin" >> "$GITHUB_PATH"
@@ -85,9 +88,11 @@ jobs:
       - name: Set up NSS/NSPR build environment (Windows)
         if: runner.os == 'Windows'
         run: |
-          echo "GYP_MSVS_OVERRIDE_PATH=$VSINSTALLDIR" >> "$GITHUB_ENV"
-          echo "GYP_MSVS_VERSION=2022" >> "$GITHUB_ENV"
-          echo "BASH=$SHELL" >> "$GITHUB_ENV"
+          {
+            echo "GYP_MSVS_OVERRIDE_PATH=$VSINSTALLDIR"
+            echo "GYP_MSVS_VERSION=2022"
+            echo "BASH=$SHELL"
+          } >> "$GITHUB_ENV"
           # See https://github.com/ilammy/msvc-dev-cmd#name-conflicts-with-shell-bash
           rm /usr/bin/link.exe
 
@@ -101,19 +106,23 @@ jobs:
         uses: ./.github/actions/nss
 
       - name: Build
-        run: cargo +${{ matrix.rust-toolchain }} build $BUILD_TYPE --all-targets --features ci
+        run: |
+          # shellcheck disable=SC2086
+          cargo +${{ matrix.rust-toolchain }} build $BUILD_TYPE --all-targets --features ci
 
       - name: Run tests and determine coverage
         run: |
+          # shellcheck disable=SC2086
           cargo +${{ matrix.rust-toolchain }} llvm-cov nextest $BUILD_TYPE --all-targets --features ci --no-fail-fast --lcov --output-path lcov.info
           cargo +${{ matrix.rust-toolchain }} bench --features bench --no-run
 
       - name: Run client/server transfer
         run: |
+          # shellcheck disable=SC2086
           cargo +${{ matrix.rust-toolchain }} build $BUILD_TYPE --bin neqo-client --bin neqo-server
-          target/$BUILD_DIR/neqo-server $HOST:4433 &
+          "target/$BUILD_DIR/neqo-server" "$HOST:4433" &
           PID=$!
-          target/$BUILD_DIR/neqo-client --output-dir . https://$HOST:4433/$SIZE
+          "target/$BUILD_DIR/neqo-client" --output-dir . "https://$HOST:4433/$SIZE"
           kill $PID
           [ "$(wc -c <"$SIZE")" -eq "$SIZE" ] || exit 1
         env:
@@ -127,6 +136,7 @@ jobs:
           if [ "${{ matrix.rust-toolchain }}" != "nightly" ]; then
             CONFIG_PATH="--config-path=$(mktemp)"
           fi
+          # shellcheck disable=SC2086
           cargo +${{ matrix.rust-toolchain }} fmt --all -- --check $CONFIG_PATH
         if: success() || failure()
 
@@ -152,10 +162,3 @@ jobs:
           fail_ci_if_error: false
           token: ${{ secrets.CODECOV_TOKEN }}
         if: matrix.type == 'debug' && matrix.rust-toolchain == 'stable'
-
-  bench:
-    name: "Benchmark"
-    needs: [check]
-    permissions:
-      pull-requests: write
-    uses: ./.github/workflows/bench.yml
diff --git a/.github/workflows/qns-comment.yml b/.github/workflows/qns-comment.yml
@@ -1,7 +1,7 @@
 # Post test results as pull request comment.
 #
 # This is done as a separate workflow as it requires write permissions. The
-# tests itself might run off of a fork, i.e. an untrusted environment and should
+# tests itself might run off of a fork, i.e., an untrusted environment and should
 # thus not be granted write permissions.
 
 name: QUIC Network Simulator Comment
@@ -21,36 +21,6 @@ jobs:
       github.event.workflow_run.event == 'pull_request' &&
       github.event.workflow_run.conclusion == 'failure'
     steps:
-      - name: Download comment-data
-        uses: actions/download-artifact@v4
+      - uses: ./.github/actions/pr-comment
         with:
-          run-id: ${{ github.event.workflow_run.id }}
-          name: comment-data
-          github-token: ${{ secrets.GITHUB_TOKEN }}
-
-      - name: Format GitHub comment
-        run: |
-          pwd
-          ls -la
-          echo '[**QUIC Interop Runner**](https://github.com/quic-interop/quic-interop-runner)' >> comment
-          echo '' >> comment
-          echo '```' >> comment
-          cat summary >> comment
-          echo '```' >> comment
-          echo '' >> comment
-          echo 'Download artifacts [here](' >> comment
-          cat logs-url >> comment
-          echo ').' >> comment
-        shell: bash
-
-      - name: Read PR Number
-        id: pr-number
-        run: echo "::set-output name=number::$(cat pr-number)"
-        shell: bash
-
-      - name: Comment PR
-        uses: thollander/actions-comment-pull-request@v2
-        with:
-          filePath: comment
-          pr_number: ${{ steps.pr-number.outputs.number }}
-          comment_tag: quic-network-simulator-comment
+          name: qns