Use unity-changeset package to scrape versions #55
Conversation
WalkthroughThe pull request updates the Changes
Poem
Warning Rate limit exceeded@AndrewKahr has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
functions/src/logic/ingestUnityVersions/scrapeVersions.ts (2)
4-4: Consider using a named regex pattern for better maintainabilityThe regex pattern correctly captures Unity version components, but readability could be improved.
Consider using a named regex pattern:
-const unity_version_regex = /^(\d+)\.(\d+)\.(\d+)([a-zA-Z]+)(-?\d+)$/; +const unity_version_regex = /^(?<major>\d+)\.(?<minor>\d+)\.(?<patch>\d+)(?<lifecycle>[a-zA-Z]+)(?<build>-?\d+)$/;This would make the version parsing more explicit and self-documenting.
9-33: Add error handling for empty or undefined unityVersionsThe current error handling could be more informative.
Consider enhancing the error handling:
-if (unityVersions?.length > 0) { +if (!unityVersions) { + throw new Error('Failed to fetch Unity versions: received undefined response'); +} + +if (unityVersions.length === 0) { + throw new Error('No Unity versions found in the response'); +} + +const parsedVersions = unityVersions .map((unityVersion) => { // ... existing mapping logic }) .filter((versionInfo): versionInfo is EditorVersionInfo => versionInfo !== null); + +if (parsedVersions.length === 0) { + throw new Error('No valid Unity versions found after filtering'); +} + +return parsedVersions; -} - -throw new Error('No Unity versions found!');functions/package.json (1)
29-29: Consider pinning the unity-changeset versionSince this package is crucial for the core functionality of scraping Unity versions, consider pinning the exact version instead of using the caret range (
^2.3.0). This would prevent unexpected breaking changes from minor version updates.- "unity-changeset": "^2.3.0" + "unity-changeset": "2.3.0"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
functions/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
functions/package.json(1 hunks)functions/src/logic/ingestUnityVersions/scrapeVersions.ts(1 hunks)functions/src/logic/utils/get-document-from-url.ts(0 hunks)
💤 Files with no reviewable changes (1)
- functions/src/logic/utils/get-document-from-url.ts
🔇 Additional comments (1)
functions/src/logic/ingestUnityVersions/scrapeVersions.ts (1)
7-7: Verify SearchMode.Default behavior
The code uses SearchMode.Default without documenting the expected behavior or potential limitations.
Let's verify the available search modes and their documentation:
| .map((unityVersion) => { | ||
| const match = RegExp(unity_version_regex).exec(unityVersion.version); | ||
| if (match) { | ||
| const [_, major, minor, patch, lifecycle, build] = match; | ||
|
|
||
| const allVersions = new Map<string, EditorVersionInfo>(); | ||
|
|
||
| for (const script of scripts) { | ||
| if (script.textContent) { | ||
| const matches = [...script.textContent.matchAll(unity_version_regex)]; | ||
| if (matches.length > 0) { | ||
| const versions = matches | ||
| .filter((match) => { | ||
| // Filter out prerelease and unsupported versions | ||
| const [_, major, minor, patch, changeSet] = match; | ||
| return patch.includes('f') && Number(major) >= 2017; | ||
| }) | ||
| .map((match) => { | ||
| const [_, major, minor, patch, changeSet] = match; | ||
| const version = `${major}.${minor}.${patch}`; | ||
| if (!allVersions.has(version)) { | ||
| return { | ||
| version, | ||
| changeSet, | ||
| major: Number(major), | ||
| minor: Number(minor), | ||
| patch, | ||
| }; | ||
| } | ||
|
|
||
| // Return null if version is not unique | ||
| if (lifecycle !== 'f' || Number(major) < 2017) { | ||
| return null; | ||
| }) | ||
| .filter((version) => version !== null) as EditorVersionInfo[]; | ||
|
|
||
| versions.forEach((it) => { | ||
| allVersions.set(it.version, it); | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (allVersions.size > 0) { | ||
| return Array.from(allVersions.values()); | ||
| } | ||
|
|
||
| return { | ||
| version: unityVersion.version, | ||
| changeSet: unityVersion.changeset, | ||
| major: Number(major), | ||
| minor: Number(minor), | ||
| patch, | ||
| } as EditorVersionInfo; | ||
| } | ||
| return null; | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving version filtering logic and error handling
The current implementation has several areas for improvement:
- The filtering logic (
lifecycle !== 'f' || Number(major) < 2017) seems to exclude final releases after 2017 - Failed regex matches silently return null
- No logging for rejected versions
Consider this improved implementation:
.map((unityVersion) => {
const match = RegExp(unity_version_regex).exec(unityVersion.version);
if (match) {
const [_, major, minor, patch, lifecycle, build] = match;
- if (lifecycle !== 'f' || Number(major) < 2017) {
+ const majorVersion = Number(major);
+ // Include final releases (f) for versions >= 2017
+ if (lifecycle === 'f' && majorVersion >= 2017) {
+ return {
+ version: unityVersion.version,
+ changeSet: unityVersion.changeset,
+ major: majorVersion,
+ minor: Number(minor),
+ patch,
+ } as EditorVersionInfo;
+ }
+ console.debug(`Skipping version ${unityVersion.version}: ${lifecycle !== 'f' ? 'non-final release' : 'old version'}`);
return null;
- }
-
- return {
- version: unityVersion.version,
- changeSet: unityVersion.changeset,
- major: Number(major),
- minor: Number(minor),
- patch,
- } as EditorVersionInfo;
}
+ console.warn(`Failed to parse version string: ${unityVersion.version}`);
return null;
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .map((unityVersion) => { | |
| const match = RegExp(unity_version_regex).exec(unityVersion.version); | |
| if (match) { | |
| const [_, major, minor, patch, lifecycle, build] = match; | |
| const allVersions = new Map<string, EditorVersionInfo>(); | |
| for (const script of scripts) { | |
| if (script.textContent) { | |
| const matches = [...script.textContent.matchAll(unity_version_regex)]; | |
| if (matches.length > 0) { | |
| const versions = matches | |
| .filter((match) => { | |
| // Filter out prerelease and unsupported versions | |
| const [_, major, minor, patch, changeSet] = match; | |
| return patch.includes('f') && Number(major) >= 2017; | |
| }) | |
| .map((match) => { | |
| const [_, major, minor, patch, changeSet] = match; | |
| const version = `${major}.${minor}.${patch}`; | |
| if (!allVersions.has(version)) { | |
| return { | |
| version, | |
| changeSet, | |
| major: Number(major), | |
| minor: Number(minor), | |
| patch, | |
| }; | |
| } | |
| // Return null if version is not unique | |
| if (lifecycle !== 'f' || Number(major) < 2017) { | |
| return null; | |
| }) | |
| .filter((version) => version !== null) as EditorVersionInfo[]; | |
| versions.forEach((it) => { | |
| allVersions.set(it.version, it); | |
| }); | |
| } | |
| } | |
| } | |
| if (allVersions.size > 0) { | |
| return Array.from(allVersions.values()); | |
| } | |
| return { | |
| version: unityVersion.version, | |
| changeSet: unityVersion.changeset, | |
| major: Number(major), | |
| minor: Number(minor), | |
| patch, | |
| } as EditorVersionInfo; | |
| } | |
| return null; | |
| }) | |
| .map((unityVersion) => { | |
| const match = RegExp(unity_version_regex).exec(unityVersion.version); | |
| if (match) { | |
| const [_, major, minor, patch, lifecycle, build] = match; | |
| const majorVersion = Number(major); | |
| // Include final releases (f) for versions >= 2017 | |
| if (lifecycle === 'f' && majorVersion >= 2017) { | |
| return { | |
| version: unityVersion.version, | |
| changeSet: unityVersion.changeset, | |
| major: majorVersion, | |
| minor: Number(minor), | |
| patch, | |
| } as EditorVersionInfo; | |
| } | |
| console.debug(`Skipping version ${unityVersion.version}: ${lifecycle !== 'f' ? 'non-final release' : 'old version'}`); | |
| return null; | |
| } | |
| console.warn(`Failed to parse version string: ${unityVersion.version}`); | |
| return null; | |
| }) |
There was a problem hiding this comment.
I'm not too sure about this suggestion, but on the other hand, the previous implementation used to be this:
patch.includes('f') && Number(major) >= 2017
So that little bunny might have found something here. Not exactly sure what are the implications tho.
There was a problem hiding this comment.
@GabLeRoux, the suggestion aims to clarify the version filtering logic and improve error handling.
In the previous implementation, the filtering condition was:
patch.includes('f') && Number(major) >= 2017In the current code, the condition is:
if (lifecycle !== 'f' || Number(major) < 2017) {
return null;
}This means that any version where the lifecycle is not 'f' (final release) or the major version is less than 2017 will be excluded.
The suggested change flips the logic to:
if (lifecycle === 'f' && majorVersion >= 2017) {
// include the version
return {
version: unityVersion.version,
changeSet: unityVersion.changeset,
major: majorVersion,
minor: Number(minor),
patch,
} as EditorVersionInfo;
}
console.debug(`Skipping version ${unityVersion.version}: ${lifecycle !== 'f' ? 'non-final release' : 'old version'}`);
return null;Implications:
- Clarity: The inclusion criteria are made explicit by checking for versions where
lifecycle === 'f'andmajorVersion >= 2017. This enhances readability and understanding. - Logging: By adding debug statements, you gain visibility into which versions are being skipped and why. This can be helpful for troubleshooting and ensuring the filtering logic is working as intended.
- Error Handling: Logging a warning when a version string fails to parse helps identify unexpected version formats returned by the
unity-changesetpackage.
Overall, the suggestion doesn't change the inclusion criteria but refines the logic for better clarity and adds helpful logging. It should function the same as the previous implementation but with improved maintainability.
Let me know if you have any questions or if you'd like to proceed with these changes!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/main.yml (1)
Line range hint
65-71: Consider optimizing cleanup steps.The cleanup of
testFunctionappears in bothtestDeployanddeployjobs. Sincedeployjob runs aftertestDeploy(as specified inneeds), andtestDeployalready includes cleanup, the second cleanup in thedeployjob might be redundant.Consider removing the redundant cleanup step from the
deployjob to simplify the workflow.deploy: name: ✨ Deploy needs: [test, build, testDeploy] runs-on: ubuntu-latest if: github.event_name == 'push' && github.ref == 'refs/heads/main' steps: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: node-version: 20 cache: 'yarn' - name: install dependencies run: yarn && yarn --cwd ./functions - name: Deploy to Firebase uses: w9jds/[email protected] with: args: deploy env: GCP_SA_KEY: '${{ secrets.FIREBASE_SERVICE_ACCOUNT_UNITY_CI_VERSIONS }}' - - name: Cleanup Firebase Test - uses: w9jds/[email protected] - if: always() - with: - args: functions:delete testFunction --force - env: - GCP_SA_KEY: '${{ secrets.FIREBASE_SERVICE_ACCOUNT_UNITY_CI_VERSIONS }}'Also applies to: 92-98
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
functions/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
.github/workflows/main.yml(3 hunks)functions/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- functions/package.json
🔇 Additional comments (1)
.github/workflows/main.yml (1)
56-56: Verify firebase-action version upgrade implications.
The upgrade from w9jds/[email protected] to v13.25.0 appears consistent across all jobs. However, it's important to verify the changes between these versions.
Also applies to: 66-66, 87-87, 93-93
✅ Verification successful
Firebase action version upgrade appears safe
The version upgrade from v13.10.2 to v13.25.0 consists of only minor version bumps of the underlying firebase-tools package, with no breaking changes or security concerns reported. All changes are routine version updates that maintain backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the changelog and releases between v13.10.2 and v13.25.0
# Fetch release information between the versions
gh api repos/w9jds/firebase-action/releases --paginate | \
jq -r '.[] | select(.tag_name | contains("v13.")) | select(.tag_name >= "v13.10.2" and .tag_name <= "v13.25.0") | "Version: \(.tag_name)\nDate: \(.published_at)\nChanges:\n\(.body)\n---"'
# Check if the new version is widely adopted (by checking recent dependent workflows)
gh api search/code -f q='path:.github/workflows w9jds/[email protected]' | \
jq -r '.items | length'
Length of output: 3785
There was a problem hiding this comment.
I did not try this locally and I'm a bit curious about the versions comparison as noted here there's a small discussion about readability: https://github.com/game-ci/versioning-backend/pull/55/files#r1845401236
I think it's better to give this a try than keeping a non-working workflow anyway. Nice work Andrew, thanks for taking the time to fix that 🙏
LGTM 🚀 
|
We got notified of these in our internal logs:
That looks good 👍
I assume this is updated metadata. I can confirm only the missing images are being built 👍 :
Nicely done |
Changes
Checklist
Summary by CodeRabbit
New Features
graphql, enhancing data handling capabilities.Improvements
Bug Fixes
Chores
firebase-action.