Deprecate Unity Editor versions pre-2021 #72
Conversation
WalkthroughThe pull request updates the Unity Ingest workflow documentation and narrows the editor version eligibility threshold from 2019+ to 2021+ for the "created" status in CI jobs. Both changes align the implementation with updated version requirements. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)functions/src/model/ciJobs.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Deploy
🔇 Additional comments (1)
README.md (1)
28-28: LGTM! Documentation accurately reflects the new behavior.The documentation update clearly specifies that only Unity versions 2021+ will have build jobs scheduled, which aligns with the code changes.
| if (editorVersionInfo === null || editorVersionInfo.major >= 2021) { | ||
| status = 'created'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify handling of existing pre-2021 jobs in the database.
The logic correctly implements the deprecation policy for NEW jobs. However, this change only affects jobs created after deployment. Existing CI jobs with status: 'created' for Unity versions 2019-2020 would remain in that state and continue to be processed by getPrioritisedQueue.
Confirm whether:
- Existing pre-2021 jobs should be automatically migrated to 'deprecated' status
- They should be allowed to complete/fail naturally
- Manual cleanup is planned
Run this script to identify any existing pre-2021 jobs that may need attention:
🏁 Script executed:
#!/bin/bash
# Description: Check for existing CI jobs with Unity versions 2019-2020 in 'created' or 'scheduled' state
# Note: This requires Firebase CLI and appropriate permissions
# Check if there are any pre-2021 jobs that might still be queued
echo "Checking for Unity 2019 jobs in active states..."
gcloud firestore documents list ciJobs --filter="editorVersionInfo.major=2019 AND (status=created OR status=scheduled OR status=inProgress)" --project=unity-ci-versions 2>/dev/null || echo "Note: Requires gcloud CLI with Firestore access"
echo ""
echo "Checking for Unity 2020 jobs in active states..."
gcloud firestore documents list ciJobs --filter="editorVersionInfo.major=2020 AND (status=created OR status=scheduled OR status=inProgress)" --project=unity-ci-versions 2>/dev/null || echo "Note: Requires gcloud CLI with Firestore access"Length of output: 194
I need to investigate the codebase further to understand the handling of existing pre-2021 jobs. Let me examine the code structure and migration mechanisms.
Let me search the codebase for any migration, cleanup, or deprecation mechanisms related to CI jobs:
🏁 Script executed:
#!/bin/bash
# Search for migration or cleanup logic related to jobs
echo "=== Searching for migration functions ==="
rg -i "migrat|deprecat|cleanup|update.*job" --type=ts functions/src/ -A3 -B1
echo ""
echo "=== Searching for status update logic ==="
rg "status.*deprecated|deprecated.*status" --type=ts functions/src/ -A2 -B2
echo ""
echo "=== Searching for pre-2021 or version checks ==="
rg "2021|2020|2019|major.*<|major.*lessthan" --type=ts functions/src/model/ciJobs.ts -A2 -B2Length of output: 3916
Let me check how jobs are queried and whether the getPrioritisedQueue function filters by status:
🏁 Script executed:
#!/bin/bash
# Check getPrioritisedQueue and how it filters jobs by status
echo "=== Searching for getPrioritisedQueue implementation ==="
rg "getPrioritisedQueue" --type=ts functions/src/ -A10 -B2
echo ""
echo "=== Searching for 'created' status query patterns ==="
rg "status.*created|'created'" --type=ts functions/src/model/ -B2 -A2
echo ""
echo "=== Check if 'deprecated' status jobs are explicitly excluded ==="
rg "status.*deprecated|'deprecated'" --type=ts functions/src/ -B3 -A3 | head -50Length of output: 4551
Implement migration logic to update existing pre-2021 jobs to 'deprecated' status.
Verification confirms the original concern is valid. The code change at lines 146-148 only affects NEW job creation—existing pre-2021 jobs with status: 'created' will continue to be processed by getPrioritisedQueue (which explicitly filters .where('status', '==', 'created')). This creates an inconsistency:
- New pre-2021 jobs:
status: 'deprecated'(excluded from queue) - Existing pre-2021 jobs:
status: 'created'(still queued and processed)
Required: Implement the dataMigrations function in functions/src/logic/dataTransformation/dataMigrations.ts to update all existing pre-2021 jobs from 'created' to 'deprecated' status. This function is already called in the cron job and is the appropriate location for this migration.
🤖 Prompt for AI Agents
In functions/src/logic/dataTransformation/dataMigrations.ts (called by the cron)
and noting the related logic in functions/src/model/ciJobs.ts around lines
146-148, implement a migration that queries all CI job documents with
editorVersionInfo.major < 2021 (or editorVersionInfo exists and major is < 2021)
AND status == 'created', and update them to status = 'deprecated'; do this
safely using batched writes (or paginated updates if >500 docs) and ensure the
function returns/awaits completion and logs counts/errors so the cron run
reliably migrates existing pre-2021 jobs that were left as 'created'.
There was a problem hiding this comment.
This is interesting, but if no pre-2021 jobs exist anymore with status created, we can probably skip the suggested migration. This can most likely be verified with a simple query in firebase.
There was a problem hiding this comment.
🧩 Analysis chain
@GabLeRoux, excellent point! If no pre-2021 jobs with status: 'created' exist in the database, the migration would indeed be unnecessary. Let me help you verify this with a Firebase query.
Once you verify that no pre-2021 jobs with status: 'created' exist, you can safely skip the migration logic. If you do find any, they would need to be migrated to 'deprecated' status to maintain consistency with the new logic.
🏁 Script executed:
#!/bin/bash
# Check for existing pre-2021 CI jobs with 'created' status in Firestore
echo "=== Attempting to query Firestore for pre-2021 jobs with 'created' status ==="
echo ""
# Try using gcloud firestore (requires gcloud CLI and authentication)
echo "Option 1: Using gcloud CLI"
echo "-------------------------"
echo "Command to check Unity 2018 jobs:"
echo "gcloud firestore documents list ciJobs --filter='editorVersionInfo.major=2018 AND status=created' --project=unity-ci-versions --format='value(name)' | wc -l"
echo ""
echo "Command to check Unity 2019 jobs:"
echo "gcloud firestore documents list ciJobs --filter='editorVersionInfo.major=2019 AND status=created' --project=unity-ci-versions --format='value(name)' | wc -l"
echo ""
echo "Command to check Unity 2020 jobs:"
echo "gcloud firestore documents list ciJobs --filter='editorVersionInfo.major=2020 AND status=created' --project=unity-ci-versions --format='value(name)' | wc -l"
echo ""
# Try using firebase CLI (requires firebase-tools and authentication)
echo "Option 2: Using Firebase CLI with Node.js script"
echo "------------------------------------------------"
cat > /tmp/check_pre2021_jobs.js << 'EOF'
// Run this with: node check_pre2021_jobs.js
// Requires: npm install firebase-admin
const admin = require('firebase-admin');
admin.initializeApp();
const db = admin.firestore();
async function checkPre2021Jobs() {
const years = [2018, 2019, 2020];
let totalCount = 0;
for (const year of years) {
const snapshot = await db.collection('ciJobs')
.where('editorVersionInfo.major', '==', year)
.where('status', '==', 'created')
.get();
console.log(`Unity ${year} jobs with status 'created': ${snapshot.size}`);
totalCount += snapshot.size;
}
console.log(`\nTotal pre-2021 jobs with 'created' status: ${totalCount}`);
return totalCount;
}
checkPre2021Jobs()
.then(() => process.exit(0))
.catch(err => {
console.error('Error:', err);
process.exit(1);
});
EOF
echo "Node.js script saved to /tmp/check_pre2021_jobs.js"
echo ""
echo "Option 3: Firebase Console UI Query"
echo "-----------------------------------"
echo "In Firebase Console > Firestore Database, run these queries:"
echo "1. Collection: ciJobs"
echo " Filter: editorVersionInfo.major == 2018 AND status == 'created'"
echo "2. Collection: ciJobs"
echo " Filter: editorVersionInfo.major == 2019 AND status == 'created'"
echo "3. Collection: ciJobs"
echo " Filter: editorVersionInfo.major == 2020 AND status == 'created'"
echo ""
echo "If all queries return 0 documents, the migration can be safely skipped."
Length of output: 1330
@GabLeRoux, I've provided three options above for verifying whether any pre-2021 jobs with status: 'created' exist in your Firestore database:
Option 1: gcloud CLI commands (fastest if you have gcloud configured)
Option 2: Node.js script using firebase-admin (saved to /tmp/check_pre2021_jobs.js)
Option 3: Firebase Console UI queries (most visual, no CLI needed)
If all queries return 0 documents, you can safely skip implementing the migration logic, as there would be no existing jobs to migrate. The code change alone is sufficient in that case.
Once you verify the results, you might want to add a comment to the PR documenting that you checked and confirmed no pre-2021 jobs with 'created' status exist, so future reviewers understand why no migration was needed.
Let me know what you find! 🔍
There was a problem hiding this comment.
I took a deeper look and indeed, we might need to handle a deprecated status since this is stateful. Coderabbit provided an outdated way to request information, but I took a look;
Details
./verify-jobs.sh
#!/bin/bash
# Check for existing pre-2021 CI jobs with 'created' status in Firestore
set -e
if [ -z "$1" ]; then
echo "Usage: ./verify-jobs.sh <project-id>"
exit 1
fi
PROJECT_ID="$1"
# Set gcloud project
gcloud config set project $PROJECT_ID --quiet
# Run the verification script
yarn node check-pre2021-jobs.js $PROJECT_ID
./check-pre2021-jobs.js
#!/usr/bin/env node
// Check for existing pre-2021 CI jobs with 'created' status in Firestore
const admin = require('firebase-admin');
const projectId = process.argv[2];
if (!projectId) {
console.error('Usage: node check-pre2021-jobs.js <project-id>');
process.exit(1);
}
// Initialize with default credentials or service account
try {
const serviceAccount = require(process.env.GOOGLE_APPLICATION_CREDENTIALS || './service-account-key.json');
admin.initializeApp({
credential: admin.credential.cert(serviceAccount),
projectId
});
} catch (error) {
admin.initializeApp({ projectId });
}
const db = admin.firestore();
async function checkPre2021Jobs() {
const years = [2018, 2019, 2020];
let totalCount = 0;
console.log('Checking pre-2021 jobs with "created" status...\n');
for (const year of years) {
const snapshot = await db.collection('ciJobs')
.where('editorVersionInfo.major', '==', year)
.where('status', '==', 'created')
.get();
console.log(`Unity ${year}: ${snapshot.size} jobs`);
totalCount += snapshot.size;
}
console.log(`\nTotal: ${totalCount} jobs`);
if (totalCount === 0) {
console.log('✓ No jobs found. Migration can be skipped.');
} else {
console.log('⚠ These jobs may need migration.');
}
}
checkPre2021Jobs()
.then(() => process.exit(0))
.catch(err => {
console.error('Error:', err.message);
console.error('\nAuthenticate with: gcloud auth application-default login');
process.exit(1);
});./verify-jobs.sh <snip (google project id)>
Updated property [core/project].
Checking pre-2021 jobs with "created" status...
Unity 2018: 76 jobs
Unity 2019: 103 jobs
Unity 2020: 61 jobs
Total: 240 jobs
⚠ These jobs may need migration.
We might have to do something about this indeed 🤔
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the dataMigrations.ts file and understand its structure
echo "=== Searching for dataMigrations.ts file ==="
fd "dataMigrations.ts" functions/src/
echo ""
echo "=== If found, show its content ==="
if fd "dataMigrations.ts" functions/src/ -q; then
fd "dataMigrations.ts" functions/src/ --exec cat {}
fi
echo ""
echo "=== Search for files in dataTransformation directory ==="
fd . "functions/src/logic/dataTransformation" --type f
echo ""
echo "=== Search for cron jobs that might call migrations ==="
rg -n "dataMigration|migration" --type=ts functions/src/ -C3
echo ""
echo "=== Search for scheduled functions or cron patterns ==="
rg -n "schedule\(|onSchedule|cron" --type=ts functions/src/ -A5 -B2
Length of output: 3343
@GabLeRoux, excellent work on verifying! With 240 existing jobs that need migration, we definitely need to implement the migration logic. Here's the implementation for functions/src/logic/dataTransformation/dataMigrations.ts that follows the existing batch update pattern from CiJobs.markJobsBeforeRepoVersionAsSuperseded:
import { db } from '../../service/firebase';
import { admin } from '../../service/firebase';
import Timestamp = admin.firestore.Timestamp;
import { chunk } from 'lodash';
import { logger } from 'firebase-functions/v2';
import { CiJob } from '../../model/ciJobs';
export const dataMigrations = async () => {
await migratePreUnity2021JobsToDeprecated();
};
/**
* Migrates all pre-2021 Unity editor jobs from 'created' status to 'deprecated'.
* This migration supports the deprecation of Unity versions before 2021.
*/
async function migratePreUnity2021JobsToDeprecated(): Promise<void> {
logger.info('Starting migration: pre-2021 jobs to deprecated status');
const yearsToMigrate = [2018, 2019, 2020];
let totalMigrated = 0;
for (const year of yearsToMigrate) {
const snapshot = await db
.collection('ciJobs')
.where('editorVersionInfo.major', '==', year)
.where('status', '==', 'created')
.get();
const count = snapshot.docs.length;
if (count === 0) {
logger.info(`Unity ${year}: No jobs to migrate`);
continue;
}
logger.info(`Unity ${year}: Migrating ${count} jobs to deprecated status`);
// Batches can only have 20 document access calls per transaction
// Note: Set counts as 2 access calls
const docsChunks = chunk(snapshot.docs, 10);
for (const docsChunk of docsChunks) {
const batch = db.batch();
for (const doc of docsChunk) {
batch.set(
doc.ref,
{
status: 'deprecated',
modifiedDate: Timestamp.now(),
},
{ merge: true }
);
}
await batch.commit();
}
totalMigrated += count;
logger.info(`Unity ${year}: Successfully migrated ${count} jobs`);
}
logger.info(`Migration complete: ${totalMigrated} total jobs migrated to deprecated status`);
}Key points:
- Follows existing patterns: Uses the same batching approach as
markJobsBeforeRepoVersionAsSuperseded(chunk size of 10, merge: true) - Safe and idempotent: Can run multiple times without issues—already-migrated jobs won't match the query
- Proper logging: Logs per-year counts and total for visibility in Cloud Functions logs
- Runs in cron: Will execute automatically on the next scheduled run after deployment
Verification after deployment:
Run your verification script again after the first cron execution to confirm all jobs were migrated:
./verify-jobs.sh <project-id>You should see ✓ No jobs found. Migration can be skipped.
Once all jobs are migrated and you've confirmed zero remain, you can optionally remove or comment out the migratePreUnity2021JobsToDeprecated() call from dataMigrations() in a follow-up PR to avoid unnecessary queries on every cron run. 🎯
🧬 Code Graph Analysis Results
-
File: functions/src/model/image.ts
- Snippet: export type ImageType = 'base' | 'hub' | 'editor';
- Line(s): 1-1
-
File: functions/src/model/editorVersionInfo.ts
-
Summary (interface):
- Description: EditorVersionInfo interface with fields:
- version: string
- changeSet: string
- major: number
- minor: number
- patch: string
- addedDate?: Timestamp
- modifiedDate?: Timestamp
- Purpose: Used to describe editor version metadata for CI jobs.
- Return/Usage: N/A (interface definition)
- Exceptions/Errors: None (type definition)
- Line range (interface): 7-15
- Description: EditorVersionInfo interface with fields:
-
Summary (class):
- Description: EditorVersionInfo class with static methods:
- collection(): returns 'editorVersions'
- get(version: string): Promise
- getAllIds(): Promise<string[]>
- getAll(): Promise<EditorVersionInfo[]>
- createMany(editorVersionList: EditorVersionInfo[]): Promise
- updateMany(versionInfoList: EditorVersionInfo[]): Promise
- Purpose: Manage CRUD/batch operations for editor version data in Firestore.
- Important details: batch operations with error logging; uses Timestamp for added/modified dates; relies on db.firestore.
- Line ranges: 17-89
- Description: EditorVersionInfo class with static methods:
-
-
File: functions/src/model/repoVersionInfo.ts
-
Summary (interface):
- Description: RepoVersionInfo interface with fields:
- id: number
- version: string
- major: number
- minor: number
- patch: number
- name: string
- description: string
- author: string
- url: string
- commitIsh: string
- addedDate?: Timestamp
- modifiedDate?: Timestamp
- Purpose: Describe repository version metadata for CI builds.
- Line range: 5-18
- Description: RepoVersionInfo interface with fields:
-
Summary (class):
- Description: RepoVersionInfo class with static methods:
- collection(): returns 'repoVersions'
- getLatest(): Promise
- getAllIds(): Promise<string[]>
- getAll(): Promise<RepoVersionInfo[]>
- create(repoVersion: RepoVersionInfo): Promise
- update(repoVersion: RepoVersionInfo): Promise
- createMany(repoVersionList: RepoVersionInfo[]): Promise
- updateMany(repoVersionList: RepoVersionInfo[]): Promise
- Purpose: Manage CRUD/batch operations for repository version data in Firestore.
- Important details: uses addedDate/modifiedDate; batch commits with error logging; sorts by major/minor/patch for latest/all.
- Line ranges: 20-131
- Description: RepoVersionInfo class with static methods:
-
-
File: functions/src/service/firebase.ts
- Summary:
- Description: Exports admin and db for Firebase Admin SDK and Firestore access; used by multiple models (e.g., CiJobs, EditorVersionInfo, RepoVersionInfo).
- Purpose: Centralize Firebase admin initialization and Firestore instance.
- Important details: likely initializes admin.app and exports db for queries; enables Firestore operations in models.
- Line range: 16-16 (references to admin and db exposure)
- Summary:
GabLeRoux
left a comment
GabLeRoux
left a comment
There was a problem hiding this comment.
See https://github.com/game-ci/versioning-backend/pull/72/files#r2554223717 we might have to do this (I'm still unsure tho).
I think the |
|
You don't have to do that if we decide to also increase the docker repo version. Might be easier than verify something in prod. |
Changes
Checklist
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.