ci: add SDK breaking change detection workflow #17075
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #17075 +/- ##
==========================================
+ Coverage 40.17% 40.44% +0.27%
==========================================
Files 3486 3513 +27
Lines 99633 100344 +711
Branches 14911 15030 +119
==========================================
+ Hits 40031 40588 +557
- Misses 57909 58045 +136
- Partials 1693 1711 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
addisonbeck
force-pushed
the
sdk-warn-on-breaking-changes
branch
4 times, most recently
from
3c9b984 to
5a5c746
Compare
addisonbeck
changed the title
Introduces GitHub Actions workflow that detects TypeScript breaking changes when SDK artifacts are updated. Workflow is triggered via repository_dispatch from SDK repository and runs npm test:types with newly built SDK artifacts. The workflow downloads SDK build artifacts, installs them locally, and executes the existing TypeScript type checking process. Exit codes determine success/failure for SDK repository monitoring via gh run watch. Addresses issue where breaking changes in SDK are discovered only when clients attempt SDK version updates, rather than during SDK development.
addisonbeck
force-pushed
the
sdk-warn-on-breaking-changes
branch
from
5a5c746 to
5ee5a4f
Compare
|
Claude finished @addisonbeck's task —— View job Code Review: SDK Breaking Change Detection WorkflowSummary of Changes Since Last ReviewThe author has addressed all of @coroiu's review feedback with the following improvements:
The workflow introduces a Critical Issues1. Missing newline at end of fileSeverity: Low The workflow file is missing a trailing newline, which violates standard text file conventions and may cause issues with some tools. Fix: Add a newline at the end of the file. Suggested Improvements1. Inconsistent step structure orderingLocation: The Azure login/logout steps are split around the GitHub App token generation step. This creates an awkward flow where Azure is logged in, then GitHub App credentials are generated, then Azure is logged out - all before the main work begins. Suggestion: Consider whether the Azure logout could be combined with the login step in a post-action, or add a comment explaining why the logout happens before checkout. Current structure- name: Log in to Azure
- name: Get Azure Key Vault secrets
- name: Generate GH App token
- name: Log out from Azure # ← Logout before main work starts
- name: Validate inputs
- name: Check out clients repository2. Retry logic discussion pointLocation: As @coroiu noted, retry logic for npm commands is not standard practice in this repository. While the author mentioned it's good practice for transient network failures, this PR introduces a pattern that doesn't exist elsewhere in the codebase. Discussion points:
Current practice: All other workflows in the repository use simple 3. Hardcoded timeout valueLocation: The Suggestion: Extract to a job-level environment variable: env:
_TYPE_CHECK_TIMEOUT_MINUTES: 10Then use: 4. Potential false negatives from timeout commandLocation: The Suggestion: Add explicit handling for timeout exit code: if timeout 10m npm run test:types; then
# success case
elif [ $? -eq 124 ]; then
echo "::error::Type checking timed out after 10 minutes"
echo "⏱️ **Result**: Type checking timed out" >> "$GITHUB_STEP_SUMMARY"
exit 1
else
# breaking changes detected case
fi5. CLIENT_LABEL validation is incompleteLocation: The validation checks if Suggestion: Either make CLIENT_LABEL truly required in the validation, or clarify in the error message which fields are required vs optional. 6. Working directory redundancyLocation: The step specifies Suggestion: Remove the Security Observations✅ Good: Secure credential handling
✅ Good: Input validation
✅ Good: Artifact verification
Performance ConsiderationsJob timeout is reasonableThe 15-minute job timeout (.github/workflows/sdk-breaking-change-check.yml:18) with a 10-minute type check timeout (.github/workflows/sdk-breaking-change-check.yml:154) provides a good balance between allowing sufficient time and preventing runaway jobs. Efficient use of GitHub Actions cacheThe workflow correctly uses Good Practices Observed
Action ItemsMust address:
Recommended: Optional: Previous Review StatusAll issues raised by @coroiu have been successfully addressed in recent commits. The workflow now follows repository conventions for artifact downloading and SDK overriding, uses proper environment variable naming, and includes appropriate error handling. The one remaining discussion point from the review is about npm ci retry logic, where the author noted it's good practice but acknowledged it's not standard in this repository. |
Add validation step early in the workflow to check for required client_payload fields and prevent failures from malformed payloads. This improvement was requested during code review to provide better error handling and debugging information when the SDK workflow sends incomplete data. Validates SOURCE_REPO, SDK_VERSION, ARTIFACTS_RUN_ID, and ARTIFACT_NAME before proceeding with artifact download and type checking.
Update GitHub Actions to consistent versions used across the clients repository for better security and compatibility. This change was requested during code review to align with existing patterns. - actions/checkout: v4.2.2 → v5.0.0 with specific SHA hash - actions/setup-node: v4.2.0 → v5.0.0 with specific SHA hash - actions/create-github-app-token: v2.1.1 → v2.0.3 with specific SHA hash Uses specific SHA hashes for all actions following repository security standards.
…tion Wrap npm run test:types with 10-minute timeout to provide faster feedback when type checking hangs and more predictable workflow behavior. This improvement was requested during code review to prevent workflows from running until the 15-minute job timeout. Provides clearer indication when type checking itself fails versus other workflow issues, improving debugging experience for developers.
Add CLIENT_LABEL to log messages and GitHub Step Summary output for better traceability and debugging. This change was requested during code review to make use of the defined CLIENT_LABEL environment variable that was previously unused. Improves workflow output clarity by showing which client type (typescript, mobile, etc.) is being processed.
Implement shell-based retry logic (3 attempts with 5-second delays) for npm ci command to handle temporary network issues without adding external dependencies. This improvement was requested during code review to make the workflow more resilient to transient failures. Continues with existing npm install approach while adding robustness for dependency installation in GitHub Actions environment.
Update shell script to use proper variable quoting syntax throughout
(${VARIABLE} instead of $VARIABLE) for better shell scripting practices
and consistency. This change was requested during code review to follow
shell scripting best practices.
While this won't cause problems in practice, it prevents potential
word splitting issues and improves code maintainability.
addisonbeck
force-pushed
the
sdk-warn-on-breaking-changes
branch
from
e37c5da to
6b2d10d
Compare
|
I'm going to leave the rest of Claude's comments on the cutting room floor. Many are false positives, and the others are nits. Even the "high" severity one seems a little silly. |
addisonbeck
requested review from
a team and
gitclonebrian
and removed request for
a team
|
@gitclonebrian it would be nice to have a BRE review on this, but not required. |
| while [ ${RETRY_COUNT} -lt ${MAX_RETRIES} ]; do | ||
| RETRY_COUNT=$((RETRY_COUNT + 1)) | ||
| echo "🔄 npm ci attempt ${RETRY_COUNT} of ${MAX_RETRIES}..." | ||
|
|
||
| if npm ci; then | ||
| echo "✅ npm ci successful" | ||
| break | ||
| else | ||
| echo "❌ npm ci attempt ${RETRY_COUNT} failed" | ||
| [ ${RETRY_COUNT} -lt ${MAX_RETRIES} ] && sleep 5 | ||
| fi | ||
| done | ||
|
|
||
| if [ ${RETRY_COUNT} -eq ${MAX_RETRIES} ]; then | ||
| echo "::error::npm ci failed after ${MAX_RETRIES} attempts" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
question: we don't usually retry npm commands do we? Is there any specific reason for doing it here?
There was a problem hiding this comment.
We don't usually do this. I think using retries to protect against transient network failures is generally a good practice. I do sometimes see timeouts from things like npm i. I think we should be doing this more as a practice, but if you think it's not worth it I won't die on this hill and will take this out.
🎟️ Tracking
Jira: https://bitwarden.atlassian.net/browse/PM-22218
Tech breakdown: https://bitwarden.atlassian.net/wiki/x/HIApfg
Calling workflow in the SDK: bitwarden/sdk-internal#538
📔 Objective
This PR introduces a GitHub Actions workflow that automatically detects TypeScript breaking changes when the SDK repository is updated. The workflow addresses the problem where SDK breaking changes are only discovered when clients attempt to update their SDK version, rather than during SDK development.
What this workflow does:
repository_dispatchevents from the SDK repository when SDK PRs are created/updatednpm run test:typescommandTechnical implementation:
test:types)gh run watchThis enables SDK developers to catch breaking changes during development rather than discovering them later during client integration.
📸 Screenshots
I temporarily hardcoded some values in pointing to a build of sdk-internal that I had intentially broken, and here is a screenshot of it failing with expected errors
The job passes pointed at a build of the sdk without breaking changes in it. Other runs can be found here but there is some debugging noise. This will be better tested once merged and available to the calling sdk job.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes