Add field to store github_branch for challenges #4745
Conversation
- Update create_or_update_github_challenge to use both github_repository and github_branch for challenge lookup - Add default branch logic: when GITHUB_REF_NAME is empty, defaults to 'challenge' - Create combined migration 0113 that adds github_branch field and unique constraint - Fix database seeder to use unique github values for each challenge - Update tests to expect 'challenge' as default branch name - Enable multiple challenge versions from same repository using different branches Fixes issue where different branches would update existing challenges instead of creating new ones.
Zahed-Riyaz
changed the title
apps/challenges/models.py
Outdated
| # The github branch name used to create/update the challenge | ||
| github_branch = models.CharField( | ||
| max_length=200, null=True, blank=True, default="" | ||
| max_length=200, null=True, blank=True, default=challenge |
There was a problem hiding this comment.
Should this not be within quotes?
apps/challenges/views.py
Outdated
| github_branch = request.data.get("GITHUB_BRANCH_NAME", "") | ||
|
|
||
| github_branch = request.data.get("GITHUB_BRANCH_NAME") or request.data.get( | ||
| "BRANCH_NAME", "challenge" |
There was a problem hiding this comment.
Can you please add a reference as to where these keys are being populated from?
There was a problem hiding this comment.
Why do we need a fallback? Should we not always expect a branch? Since the challenge repo will be pushing at some branch?
There was a problem hiding this comment.
I don't believe there's a reference of the keys in EvalAI documentation, but originally I used GITHUB_REF_NAME and was instructed to change it to GITHUB_BRANCH_NAME during PR review in #4737.
I updated the empty string default to "challenge" now for backward compatibility so that old existing challenges (I believe in local servers) will now have a github_branch versus an empty string. Any challenge on the platform would naturally always expect a github_branch and will store one.
Modern workflows should always specify the branch, older scripts and code if for whatever reason don't specify the github_branch, they'll have a default fallback for good measure.
| # Data migration to populate existing records | ||
| migrations.RunPython( | ||
| populate_github_branch_default, | ||
| reverse_populate_github_branch_default, | ||
| ), |
There was a problem hiding this comment.
Why are we running populate github branch default and then reversing it??
There was a problem hiding this comment.
The migration itself was written to handle the populating at first (then reverse it for use of script), but now that a script is explicitly written for it, it might be best to remove it from the migration entirely.
| # Data migration to populate existing records | ||
| migrations.RunPython( | ||
| populate_github_branch_default, | ||
| reverse_populate_github_branch_default, | ||
| ), |
There was a problem hiding this comment.
Is this the best way to handle this scenario?
| #!/usr/bin/env python | ||
| # Command to run: python manage.py shell < scripts/migration/populate_github_branch.py | ||
| """ | ||
| Populate existing challenges with github_branch="challenge" for backward compatibility. | ||
| This script should be run after the migration to ensure all existing challenges | ||
| have the github_branch field populated with the default value. | ||
| """ | ||
|
|
||
| import traceback | ||
|
|
||
| from challenges.models import Challenge | ||
| from django.db import models | ||
|
|
||
|
|
||
| def populate_github_branch_fields(): | ||
| """ | ||
| Populate existing challenges with empty github_branch fields to use "challenge" as default. | ||
| """ | ||
| print("Starting github_branch field population...") | ||
|
|
||
| challenges_to_update = ( | ||
| Challenge.objects.filter(github_repository__isnull=False) | ||
| .exclude(github_repository="") | ||
| .filter( | ||
| models.Q(github_branch__isnull=True) | models.Q(github_branch="") | ||
| ) |
There was a problem hiding this comment.
If we already have this script to populate and back-fill, why do we need to the populate/reverse populate methods at all?
| ) or self.request.data.get("BRANCH_NAME") | ||
| if not branch: | ||
| branch = "challenge" | ||
| pattern = r"^challenge-\d{4}-[a-zA-Z0-9]+$" |
There was a problem hiding this comment.
Will this also be okay with just "challenge" (default) if passed?
tests/unit/challenges/test_views.py
Outdated
| self.assertEqual(response.json(), expected) | ||
|
|
||
| # Verify github_branch defaults to empty string when not provided | ||
| # Verify github_branch defaults to "challenge" when not provided |
There was a problem hiding this comment.
Please also add when the branch name is "challenge", when branch name is "xyzabc", and when branch name is "challenge-2025-v2" (whatever format you have chosen).
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4745 +/- ##
==========================================
+ Coverage 72.93% 76.91% +3.98%
==========================================
Files 83 21 -62
Lines 5368 3608 -1760
==========================================
- Hits 3915 2775 -1140
+ Misses 1453 833 -620 see 75 files with indirect coverage changes see 75 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
The migration as of now adds This is why changes were made in |
This PR amends #4737 to store github_branch field for challenge creation.