Skip to content

Add integ workflow - #193

Open
simonmarty wants to merge 6 commits into
aws:masterfrom
simonmarty:master
Open

Add integ workflow#193
simonmarty wants to merge 6 commits into
aws:masterfrom
simonmarty:master

Conversation

@simonmarty

@simonmarty simonmarty commented Mar 31, 2026

Copy link
Copy Markdown
Contributor

Description

Why is this change being made?

We do not have a way to run integration tests automatically.

What is changing?

  1. Add integ workflow
  2. Add workflow to remove the safe-to-test label when new PRs are pushed.

Related Links

  • Issue #, if available:

Testing

How was this tested?

  1. Tested on fork.

When testing locally, provide testing artifact(s):

  1. https://github.com/simonmarty/aws-secretsmanager-caching-net/actions/runs/23774021104

Reviewee Checklist

Update the checklist after submitting the PR

  • I have reviewed, tested and understand all changes
    If not, why:
  • I have filled out the Description and Testing sections above
    If not, why:
  • Build and Unit tests are passing
    If not, why:
  • Unit test coverage check is passing
    If not, why:
  • Integration tests pass locally
    If not, why:
  • I have updated integration tests (if needed)
    If not, why:
  • I have ensured no sensitive information is leaking (i.e., no logging of sensitive fields, or otherwise)
    If not, why:
  • I have added explanatory comments for complex logic, new classes/methods and new tests
    If not, why:
  • I have updated README/documentation (if needed)
    If not, why:
  • I have clearly called out breaking changes (if any)
    If not, why:

Reviewer Checklist

All reviewers please ensure the following are true before reviewing:

  • Reviewee checklist has been accurately filled out
  • Code changes align with stated purpose in description
  • Test coverage adequately validates the changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@simonmarty
simonmarty requested a review from a team as a code owner March 31, 2026 00:13
@codecov

codecov Bot commented Mar 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.92%. Comparing base (e4484aa) to head (123e20a).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #193      +/-   ##
==========================================
+ Coverage   73.18%   76.92%   +3.74%     
==========================================
  Files           5        5              
  Lines         220      234      +14     
  Branches       23       24       +1     
==========================================
+ Hits          161      180      +19     
+ Misses         50       46       -4     
+ Partials        9        8       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@simonmarty simonmarty changed the title Matrix dotnet build, add integ workflow Add integ workflow Apr 2, 2026
@secrets-manager-ai-code-review

secrets-manager-ai-code-review Bot commented Jul 8, 2026

Copy link
Copy Markdown

Claude finished @simonmarty's task in 1m 24s —— View job


PR Review: Add integ workflow

  • Read changed workflow files
  • Analyze integ.yml for correctness/security
  • Analyze pr-sync.yml for correctness/security
  • Post review feedback

This is a solid addition — gating pull_request_target behind a safe-to-test label and stripping that label on every synchronize is the correct pattern for safely running integration tests with cloud credentials against fork PRs. A few issues below, one of which is a real bug.


🔴 Bug — matrix.dotnet-version is never defined

.github/workflows/integ.yml:24

- name: Install .NET
  uses: actions/setup-dotnet@v5
  with:
    dotnet-version: ${{ matrix.dotnet-version }}

There is no strategy.matrix block on the integ job, so matrix.dotnet-version resolves to an empty string. setup-dotnet will then fall back to global.json / latest behavior rather than the intended version. The integ project targets net8.0;net48, so this should mirror dotnet.yml:

    dotnet-version: |
      8

Fix this →


🟡 pr-sync.yml — the status-check case logic is effectively dead

.github/workflows/pr-sync.yml:21-26

gh_status=$(gh api "repos/$REPO/issues/$PR_NUMBER/labels/$LABEL_NAME" -X DELETE | jq 'if type == "object" then .status else empty end' --raw-output)
  • On success, the API returns the remaining-labels array, so the jq type == "object" branch yields empty → "Label removed". ✅
  • On 404 (label already absent), the error body is {"message": ..., "documentation_url": ...} — it has no .status field, so .status is null/empty and you again land in the """Label removed" branch. The 404) and *) cases never actually fire.
  • Because the pipeline's exit code is jq's (not gh's), a genuine gh failure is also swallowed.

Net effect: removing a non-existent label is silently treated as success, which is the desired outcome, but the 404/error handling is illusory. Consider simplifying to something honest, e.g.:

gh api "repos/$REPO/issues/$PR_NUMBER/labels/$LABEL_NAME" -X DELETE --silent \
  || echo "Label not present (or already removed) — ignoring"

🟡 Redundant if: runner.os == 'Windows'

.github/workflows/integ.yml:26runs-on is hard-coded to windows-latest, so this guard is always true. Harmless, but it reads as leftover from a matrix that no longer exists (same root cause as the matrix.dotnet-version reference).


🟢 Minor / nits

  • Action version drift: integ.yml pins checkout@v6, setup-dotnet@v5, setup-msbuild@v3, configure-aws-credentials@v6, while dotnet.yml uses @v4/@v4/@v2. Not blocking, but worth aligning the two workflows (and double-checking those major tags actually exist) so behavior stays consistent.
  • No concurrency group: rapid re-labeling could launch overlapping integ runs. Adding a concurrency block keyed on the PR would cancel superseded runs.
  • Security (looks good): id-token: write + contents: read is appropriately scoped, the checkout pins github.event.pull_request.head.sha, and the label-add / label-remove-on-push handshake closes the usual pull_request_target TOCTOU gap. Just make sure the safe-to-test label can only be applied by maintainers (default for collaborator-only label permissions) — the whole safety model rests on that.

Note: I can review these workflow files but can't modify files under .github/workflows/ myself due to GitHub App permission restrictions — the matrix.dotnet-version fix will need to be pushed by a maintainer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant