[PM-13621] ci: Add cron job to regularly update SPM dependencies in project files - #1753
KatherineInCode wants to merge 27 commits into
Conversation
|
Great job, no security vulnerabilities found in this Pull Request |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1753 +/- ##
===========================================
- Coverage 87.20% 41.18% -46.03%
===========================================
Files 1895 592 -1303
Lines 167767 32503 -135264
===========================================
- Hits 146304 13385 -132919
+ Misses 21463 19118 -2345 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
vvolkgang
left a comment
vvolkgang
left a comment
There was a problem hiding this comment.
Consolidating what we discussed and some review notes:
- xcodegen project files will be the source of truth and we should untrack
Package.resolved, we'll use xcodegen project files for caching purposes - We need to pin dependencies to
revision:- for human review purposes confirm if we can continue having theexactVersion:setting inproject-*.ymlfiles, otherwise we can add a comment next to the revision hash with the release tag name, example. - Replace GitHub API curl calls with GH CLI - for examples search for GH_TOKEN in our repo.
- Consider using python (with classes) instead of bash for the script, they've been easier to maintain and improve. I can help you set it up with uv.
- Listing updated packages in the PR description - I can help with that, we can try mimicking some of the Renovate PRs structure. 🤔
b187bbd to
e916ed5
Compare
- Refactor update-dependencies.py with GitHubClient, ProjectFileUpdater, and DependencyUpdateRunner classes - Write spm-update-summary.md with a package update table; use it as PR body - Fix broken $PSL_FILE references in workflow; track project YAML files instead - Add pip install step for pyyaml and packaging - Untrack Package.resolved and uncomment it in .gitignore - Remove update-dependencies.sh (superseded by Python script)
KatherineInCode
changed the title
KatherineInCode
added
ai-review-vnext
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Reviewed the new weekly cron workflow that runs Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| def _update_revision( | ||
| self, | ||
| name: str, | ||
| info: object, | ||
| url: str, | ||
| current: str, | ||
| branch: str, | ||
| ) -> Optional[PackageUpdate]: | ||
| """Check for and apply a newer commit revision. | ||
|
|
||
| The inline comment on the ``revision`` key (if any) is preserved as-is, | ||
| since the script has no way to determine the human-readable version | ||
| identifier for an arbitrary commit SHA. | ||
|
|
||
| Args: | ||
| name: Package name (for logging). | ||
| info: Package CommentedMap; ``revision`` is mutated on update. | ||
| url: GitHub repository URL. | ||
| current: Current revision SHA. | ||
| branch: Branch to query for the latest commit. | ||
|
|
||
| Returns: | ||
| A PackageUpdate if a newer commit was found and applied, else None. | ||
| """ | ||
| latest = self.client.get_latest_commit(url, branch) | ||
| if latest and latest != current: | ||
| print(f" Updating {name} revision: {current[:8]}… → {latest[:8]}…") | ||
| info["revision"] = latest | ||
| return PackageUpdate( | ||
| package_name=name, | ||
| source_file=self.path, | ||
| old_value=current, | ||
| new_value=latest, | ||
| update_kind="revision", | ||
| ) | ||
| print(f" {name} revision is up to date ({current[:8]}…)") | ||
| return None |
There was a problem hiding this comment.
❓ QUESTION: After an exactVersion package is converted to revision/branch, subsequent cron runs follow branch HEAD rather than the next stable tag — was this intentional?
Details
_process_package only enters _convert_version while exactVersion is still present. Once converted, every later run takes the _update_revision path, which calls get_latest_commit(url, branch) and pins to whatever commit is currently at the head of the default branch.
Two follow-on effects:
- Packages migrate from "stable releases only" to "main branch HEAD" tracking after the first conversion. For example,
FirebaseatexactVersion: 11.14.0would convert to that tag's SHA, but the next cron run would jump to whatever commit landed onfirebase-ios-sdk's default branch — potentially ahead of any tagged release. The cron will never re-pin to a newer stable tag (e.g. 11.15.0) onceexactVersionis gone. - The inline tag comment added by
info.yaml_add_eol_comment(target_tag, "revision")is preserved as-is by_update_revision(per the docstring), so arevision: <sha> # 11.14.0line can drift to point at a commit that has nothing to do with 11.14.0, and reviewers of generated PRs lose the human-readable version signal.
If the intent is to track stable releases only, _update_revision could re-query get_latest_release + get_tag_commit_sha for converted packages and refresh the inline comment when the tag changes. If branch-HEAD tracking is the goal for these packages, it's worth calling out in the PR description so future maintainers (and reviewers of the generated PRs) understand what the comment represents.
| is_version_bump = _is_older(current, latest_tag) | ||
| target_tag = latest_tag | ||
|
|
||
| sha = self.client.get_tag_commit_sha(url, target_tag) | ||
| if sha is None: | ||
| print(f" {name}: Could not resolve SHA for {target_tag}, skipping...") | ||
| return None | ||
|
|
||
| default_branch = self.client.get_default_branch(url) | ||
|
|
||
| if is_version_bump: | ||
| print(f" Converting and updating {name}: {current} → {target_tag} ({sha[:8]}…)") | ||
| update_kind = "version" | ||
| else: | ||
| print(f" Converting {name} {current} to revision format ({sha[:8]}…)") | ||
| update_kind = "conversion" | ||
|
|
||
| del info["exactVersion"] | ||
| info["revision"] = sha | ||
| info.yaml_add_eol_comment(target_tag, "revision") |
There was a problem hiding this comment.
❓ QUESTION: The docstring says "otherwise the current version is re-pinned at its commit SHA," but target_tag = latest_tag is unconditional — was this intentional?
Details
The docstring (lines 306–308) describes a two-branch behavior:
If the latest release is newer than current, the new tag is used; otherwise the current version is re-pinned at its commit SHA.
But in the code, target_tag is always set to latest_tag regardless of is_version_bump. The SHA fetched on line 330, the comment written on line 346, and the new_value returned on line 353 all reflect latest_tag, not current.
Concrete scenario where this diverges from the docstring: a developer manually pins exactVersion: 12.0.0-beta1 (e.g., to consume a fix that hasn't shipped in a stable release yet). On the next cron run:
latest_tag = "11.14.0"(the latest stable)_is_older("12.0.0-beta1", "11.14.0")→False(12.0.0-beta1 is newer per PEP 440)is_version_bump = False, the log says "Converting 12.0.0-beta1 to revision format…"- But
target_tag = "11.14.0", so the package is silently downgraded to 11.14.0's SHA.
The current exactVersion packages (Firebase 11.14.0, SwiftProtobuf 1.31.0, SwiftUIIntrospect 1.3.0, SnapshotTesting 1.18.4, ViewInspector 0.10.3) are all probably behind their latest stable, so this won't trigger on the first run — but the divergence from the docstring is worth resolving either by branching target_tag based on is_version_bump, or by updating the docstring to reflect that the script always converts to latest_tag.
| - name: Run update script | ||
| run: ./Scripts/update-dependencies.py |
There was a problem hiding this comment.
gh api calls but this step does not set GH_TOKEN/GITHUB_TOKEN, so authentication will fail.
Details and fix
./Scripts/update-dependencies.py shells out to gh api for releases, tags, default branch, and commits — roughly a dozen calls per run. The gh CLI on GitHub-hosted runners is not pre-authenticated; it requires GH_TOKEN (or GITHUB_TOKEN) in the step's environment, which other workflows in this repo set explicitly (e.g. sdlc-sdk-update.yml lines 96, 126, 153). Unauthenticated gh api calls will either fail with gh: To authenticate, please run... or, for public-only endpoints, be capped at the 60-request/hour anonymous rate limit.
Because _call_api now raises RuntimeError on failure (and main() exits with status 1), the workflow step will fail loudly every Monday and the downstream "Check for changes" / commit / PR steps (gated on has_changes == 'true') will never run.
| - name: Run update script | |
| run: ./Scripts/update-dependencies.py | |
| - name: Run update script | |
| env: | |
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| run: ./Scripts/update-dependencies.py |
e06bb2e to
0c9748e
Compare
|
Closing in favor of #2679 |
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-13621
📔 Objective
This creates a new cron job in the pattern of our "update public suffix list" job that checks the SPM packages defined in our project files for updates, and if they exist creates a PR for it. The
update-dependencies.shscript can also be run offline if desired.This should hopefully let us keep better tabs on those updates, and keep our dependencies up to date.
This has also been an opportunity for me to experiment with Claude Code, in terms of writing the bash script.
⏰ 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