Add LVBS Smoke Test to pull requests - #1458
Praveen K Paladugu (praveen-pk) wants to merge 1 commit into
Conversation
|
#1344 has a sample run of the LVBS Smoke Pipeline. |
Trigger the LVBS Smoke Test pipeline for pull requests against main branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ff2f379 to
7c45e8c
Compare
|
🤖 SemverChecks 🤖 No semver-relevant crate changes detected; skipped cargo-semver-checks. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The privileged workflow needs an approval boundary and an immutable Azure Login action reference.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Adds an LVBS smoke-test workflow for pull requests targeting main.
Changes:
- Triggers and monitors the Azure DevOps LVBS pipeline.
- Cancels superseded builds.
| File | Description |
|---|---|
.github/workflows/lvbs-smoke-test.yml |
Defines the PR-triggered LVBS smoke test. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| name: LVBS Smoke Test | ||
|
|
||
| on: | ||
| pull_request_target: |
| timeout-minutes: 180 | ||
| steps: | ||
| - name: Azure login | ||
| uses: azure/login@v2 |
There was a problem hiding this comment.
Yep, pinning the action and then running this script to confirm that the hash is valid is a good idea:
grep -rhoP 'uses:\s*\K[\w.-]+/[\w.-]+@[0-9a-f]+\s*#\s*v?[\w.-]+' .github/workflows/ \
| sed 's/#//' | sort -u \
| while read -r ref tag; do
repo=${ref%@*}; sha=${ref#*@}
actual=$(gh api "repos/$repo/git/ref/tags/$tag" --jq '.object.sha' 2>/dev/null)
typ=$(gh api "repos/$repo/git/ref/tags/$tag" --jq '.object.type' 2>/dev/null)
[ "$typ" = tag ] && actual=$(gh api "repos/$repo/git/tags/$actual" --jq '.object.sha')
if [ "$actual" = "$sha" ]; then echo "OK $repo $tag"; else echo "BAD $repo $tag: comment=$tag sha=$sha actual=$actual"; fi
done
Jay Bosamiya (Microsoft) (jaybosamiya-ms)
left a comment
Jay Bosamiya (Microsoft) (jaybosamiya-ms)
left a comment
There was a problem hiding this comment.
Broadly seems reasonable to me, thanks Praveen! I do have a couple of questions though, and the biggest concern is around the pull_request_target which is a somewhat scary option given that we have secrets involved.
| name: LVBS Smoke Test | ||
|
|
||
| on: | ||
| pull_request_target: |
There was a problem hiding this comment.
The pull_request_target has me a bit concerned me, do we really need it to be pull_request_target? Can we instead have it be pull_request? If we make it pull_request, it will not run on PRs made by people external to the org, but it becomes a lot easier to reason about access to the things. Also, the PR that causes a version bump will be from folks internal to the project, so will not be from a fork, so just pull_request should be sufficient imho.
| lvbs_smoke_test: | ||
| name: LVBS Smoke Test | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 180 |
There was a problem hiding this comment.
I think you mentioned average run time is ~15 minutes, this is a huge amount of extra time we are giving for stuff to run without being noticed. Maybe we should set it to 30 or 45 minutes?
| timeout-minutes: 180 | ||
| steps: | ||
| - name: Azure login | ||
| uses: azure/login@v2 |
There was a problem hiding this comment.
Yep, pinning the action and then running this script to confirm that the hash is valid is a good idea:
grep -rhoP 'uses:\s*\K[\w.-]+/[\w.-]+@[0-9a-f]+\s*#\s*v?[\w.-]+' .github/workflows/ \
| sed 's/#//' | sort -u \
| while read -r ref tag; do
repo=${ref%@*}; sha=${ref#*@}
actual=$(gh api "repos/$repo/git/ref/tags/$tag" --jq '.object.sha' 2>/dev/null)
typ=$(gh api "repos/$repo/git/ref/tags/$tag" --jq '.object.type' 2>/dev/null)
[ "$typ" = tag ] && actual=$(gh api "repos/$repo/git/tags/$actual" --jq '.object.sha')
if [ "$actual" = "$sha" ]; then echo "OK $repo $tag"; else echo "BAD $repo $tag: comment=$tag sha=$sha actual=$actual"; fi
done
| - name: Cancel superseded ADO pipeline | ||
| if: cancelled() && env.ADO_BUILD_ID != '' | ||
| shell: bash | ||
| env: |
There was a problem hiding this comment.
Does a timeout on the github side send a cancellation to the azure side? This is more a question of sanity checking that we don't accidentally have super long runaways on that side (if there is already a timeout on that end, we don't need to worry here)
|
Oh and the unrelated CI failures are due to a recent Rust version update, Weiteng is fixing them up in #1468 but you may need to rebase once that is merged, just to get the CI to be happy here |
Trigger the LVBS Smoke Test pipeline for pull requests against main branch.