feat: Implement bi-directional GitHub sync for challenges (#4760)#1
Conversation
This implements the bi-directional GitHub sync feature that allows changes made through the EvalAI UI to be automatically synced back to the linked GitHub repository. Closes Cloud-CV#4760
There was a problem hiding this comment.
Pull request overview
This PR implements bi-directional GitHub synchronization for challenges, allowing changes made through the EvalAI web UI to be automatically pushed back to the linked GitHub repository. This addresses the limitation where EvalAI previously only supported one-way sync (GitHub → EvalAI).
Key changes:
- Added
github_tokenfield to store GitHub Personal Access Token for API authentication - Implemented Django post_save signals that trigger Celery tasks for asynchronous GitHub synchronization
- Created
GithubInterfaceclass to handle GitHub API communication (read/write file content, update YAML configs)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
apps/challenges/migrations/0114_add_github_token_field.py |
Database migration adding github_token field to Challenge model for storing GitHub PAT |
apps/challenges/models.py |
Added github_token field and post_save signal handlers for Challenge and ChallengePhase to trigger GitHub sync |
apps/challenges/serializers.py |
Exposed github_token field in ChallengeSerializer and ZipChallengeSerializer for API access |
apps/challenges/github_utils.py |
Celery tasks for async sync, loop prevention using thread-local storage, and helper functions for field change detection |
apps/challenges/github_sync_config.py |
Configuration defining which Challenge and ChallengePhase fields should be synced to GitHub |
apps/challenges/github_interface.py |
Interface class for GitHub API communication handling file read/write and YAML config updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not changed_field: | ||
| logger.debug( | ||
| f"Could not determine changed field for challenge phase " | ||
| f"{challenge_phase.pk}" | ||
| ) | ||
| return |
There was a problem hiding this comment.
Similar to the Challenge sync, when changed_field cannot be determined for ChallengePhase updates, the sync is silently skipped. This can lead to data inconsistency when multiple phase fields are updated simultaneously.
Consider:
- Supporting multi-field updates
- Logging at warning level when sync is skipped
- Providing fallback behavior for bulk updates
This issue compounds with the Challenge sync issue - if a user updates multiple challenge and phase fields together, none of the changes will propagate to GitHub.
| logger.info( | ||
| "EvalAI is not able to establish connection with github {}".format( | ||
| response.json() if response else "No response" | ||
| ) | ||
| ) | ||
| return None |
There was a problem hiding this comment.
The error logging message attempts to include response.json() but if the request fails before getting a response (e.g., network error, DNS failure), response will be None and this will log "No response". However, the actual exception that caused the failure is swallowed and not logged, making debugging difficult.
Consider:
except requests.exceptions.RequestException as e:
logger.error(
f"Failed to connect to GitHub API: {str(e)}. "
f"Response: {response.json() if response else 'No response'}"
)
return NoneThis provides better context about why the request failed.
| new_text = self._read_text_from_file_field( | ||
| getattr(challenge, changed_field, None) | ||
| ) | ||
| if new_text is None or new_text == current_text: |
There was a problem hiding this comment.
The function reads the text from a file field and compares it to the current GitHub content. If they're equal or if new_text is None, it returns True indicating success. However, returning True when new_text is None could mask errors where the file couldn't be read. This makes it appear as if the sync succeeded when it actually failed to read the file.
Consider returning False when new_text is None and logging an appropriate error message, so that sync failures are properly reported.
| if new_text is None or new_text == current_text: | |
| if new_text is None: | |
| logger.error( | |
| f"Failed to read new text for field '{changed_field}' from file field; sync failed." | |
| ) | |
| return False | |
| if new_text == current_text: |
| @shared_task | ||
| def github_challenge_phase_sync( | ||
| serialized_challenge_phase, serialized_challenge, changed_field | ||
| ): | ||
| """ | ||
| Celery task to sync a ChallengePhase change to GitHub. | ||
|
|
||
| Args: | ||
| serialized_challenge_phase: JSON serialized ChallengePhase object | ||
| serialized_challenge: JSON serialized Challenge object (parent) | ||
| changed_field: The field that was changed | ||
| """ | ||
| try: | ||
| challenge_phase = deserialize_object(serialized_challenge_phase) | ||
| challenge = deserialize_object(serialized_challenge) | ||
|
|
||
| if not challenge_phase or not challenge: | ||
| logger.error( | ||
| "Failed to deserialize challenge_phase or challenge for GitHub sync" | ||
| ) | ||
| return False | ||
|
|
||
| # Verify GitHub config is present on parent challenge | ||
| if not challenge.github_repository or not challenge.github_token: | ||
| logger.debug( | ||
| f"Challenge {challenge.pk} missing github_repository or github_token, " | ||
| "skipping phase sync" | ||
| ) | ||
| return False | ||
|
|
||
| # Create GitHub interface | ||
| github = GithubInterface( | ||
| challenge.github_repository, | ||
| challenge.github_branch, | ||
| challenge.github_token, | ||
| ) | ||
|
|
||
| # Verify repository access | ||
| if not github.is_repository(): | ||
| logger.error( | ||
| f"Cannot access GitHub repository: {challenge.github_repository}" | ||
| ) | ||
| return False | ||
|
|
||
| # Sync the changed field | ||
| success = github.update_challenge_phase_config(challenge_phase, changed_field) | ||
| if success: | ||
| logger.info( | ||
| f"Successfully synced {changed_field} for challenge phase " | ||
| f"{challenge_phase.pk}" | ||
| ) | ||
| else: | ||
| logger.error( | ||
| f"Failed to sync {changed_field} for challenge phase " | ||
| f"{challenge_phase.pk}" | ||
| ) | ||
| return success | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Error in github_challenge_phase_sync: {str(e)}") | ||
| return False | ||
|
|
There was a problem hiding this comment.
The github_challenge_phase_sync task lacks test coverage. This function handles synchronization of challenge phase data to GitHub and needs comprehensive testing.
Tests should cover:
- Successful phase sync scenarios
- Proper handling of parent challenge relationships
- Different phase field types
- Error scenarios
- Verification that changes are correctly written to the YAML config
Consider adding tests in a new test_github_utils.py file.
| github_token = models.CharField( | ||
| max_length=500, null=True, blank=True, default="" | ||
| ) |
There was a problem hiding this comment.
The github_token is stored as plain text in the database. This is a security risk as GitHub Personal Access Tokens are sensitive credentials that should be encrypted at rest.
Recommendations:
- Use Django's encryption libraries (e.g.,
django-fernet-fieldsordjango-cryptography) to encrypt the token before storing it - Alternatively, use environment variables or a secrets management service (like AWS Secrets Manager, HashiCorp Vault) and store only a reference ID
- Ensure the database backups are also properly secured since they contain these tokens
Plain text storage of authentication tokens violates security best practices and could lead to unauthorized access if the database is compromised.
| @receiver(signals.post_save, sender="challenges.ChallengePhase") | ||
| def challenge_phase_details_sync(sender, instance, created, **kwargs): | ||
| """ | ||
| Signal to sync ChallengePhase changes to GitHub repository. | ||
| Triggered on post_save when a ChallengePhase is updated. | ||
| """ | ||
| # Skip on create - only sync updates | ||
| if created: | ||
| return | ||
|
|
||
| # Get parent challenge | ||
| challenge = instance.challenge | ||
|
|
||
| # Skip if no GitHub config on parent challenge | ||
| if not challenge.github_repository or not challenge.github_token: | ||
| return | ||
|
|
||
| # Import here to avoid circular imports | ||
| from challenges.github_utils import trigger_challenge_phase_sync | ||
|
|
||
| # Get update_fields from kwargs if available | ||
| update_fields = kwargs.get("update_fields") | ||
| trigger_challenge_phase_sync(instance, update_fields) | ||
|
|
There was a problem hiding this comment.
The challenge_phase_details_sync signal handler lacks test coverage. This signal is responsible for triggering phase syncs and should be tested to ensure:
- Fires on ChallengePhase updates (but not creation)
- Correctly accesses parent challenge configuration
- Skips when GitHub config is missing
- Properly invokes the trigger function
- Handles edge cases like orphaned phases
Consider adding comprehensive signal tests.
| if changed_field in {"evaluation_script"}: | ||
| file_path = config_data.get(changed_field) | ||
| if not file_path: | ||
| logger.warning( | ||
| f"No path for '{changed_field}' in challenge_config.yaml; " | ||
| "skipping file update" | ||
| ) | ||
| return False | ||
| current_text = self.get_data_from_path(file_path) | ||
| new_text = self._read_text_from_file_field( | ||
| getattr(challenge, changed_field, None) | ||
| ) | ||
| if new_text is None or new_text == current_text: | ||
| return True | ||
| return ( | ||
| True | ||
| if self.update_data_from_path(file_path, new_text, changed_field) | ||
| else False | ||
| ) |
There was a problem hiding this comment.
The function updates file-based fields by finding the file path in the existing config and updating that file. However, if the field exists in CHALLENGE_SYNC_FIELDS but the path isn't in the config, the function logs a warning and returns False, which could be confusing.
Additionally, there's no validation that the field is actually in CHALLENGE_FILE_FIELDS before treating it as a file field. Consider:
- Adding explicit validation:
if changed_field not in CHALLENGE_FILE_FIELDS: ... - Providing clearer error messages about what's missing
- Potentially creating the file if it doesn't exist rather than failing
This would make the API more robust and self-documenting.
| null=True, | ||
| blank=True, | ||
| default="", | ||
| help_text="GitHub Personal Access Token for bi-directional sync", |
There was a problem hiding this comment.
The migration includes a help_text parameter that doesn't appear in the model definition at apps/challenges/models.py:194-196. This inconsistency means the help text will appear in the database schema but not be accessible through Django's ORM or admin interface.
If the help text is intentional, it should also be added to the model field definition:
github_token = models.CharField(
max_length=500,
null=True,
blank=True,
default="",
help_text="GitHub Personal Access Token for bi-directional sync"
)This ensures consistency between migrations and models and makes the help text available in Django admin and form generation.
| "cli_version", | ||
| "github_repository", | ||
| "github_branch", | ||
| "github_token", |
There was a problem hiding this comment.
The github_token field is being serialized and exposed through the API in ZipChallengeSerializer. This is a critical security vulnerability as it exposes the GitHub Personal Access Token to API consumers. GitHub PATs should never be returned in API responses.
Consider either:
- Adding
github_tokento awrite_only_fieldslist or usingwrite_only=Truein the serializer field definition - Excluding it from the serialized output entirely
- Using Django's
extra_kwargs = {'github_token': {'write_only': True}}in the Meta class
The token should only be accepted during creation/update but never returned in read operations.
| # Determine which field changed | ||
| changed_field = get_changed_field_from_update_fields( | ||
| update_fields, CHALLENGE_SYNC_FIELDS | ||
| ) | ||
|
|
||
| # If not from update_fields, try payload keys | ||
| if not changed_field: | ||
| changed_field = get_changed_field_from_payload( | ||
| get_payload_keys(), CHALLENGE_SYNC_FIELDS | ||
| ) | ||
|
|
||
| if not changed_field: | ||
| logger.debug( | ||
| f"Could not determine changed field for challenge {challenge.pk}" | ||
| ) | ||
| return | ||
|
|
||
| # Mark as synced to prevent re-entry | ||
| mark_synced("Challenge", challenge.pk) | ||
|
|
||
| # Serialize and queue the sync task | ||
| serialized_challenge = serializers.serialize("json", [challenge]) | ||
| github_challenge_sync.delay(serialized_challenge, changed_field) | ||
| logger.info( | ||
| f"Queued GitHub sync for challenge {challenge.pk}, field: {changed_field}" | ||
| ) |
There was a problem hiding this comment.
When changed_field cannot be determined, the function logs and returns early without syncing. However, if multiple fields are updated simultaneously (which is common in bulk updates or form submissions), the sync will be silently skipped. This could lead to data inconsistency between EvalAI and GitHub.
Consider:
- Supporting multi-field updates by syncing all changed syncable fields
- At minimum, logging at a higher level (warning) when sync is skipped due to multiple field changes
- Adding configuration to optionally sync the entire config when multiple fields change
Example scenario: If a challenge organizer updates both title and description in the UI at once, neither change will be synced to GitHub.
| # Determine which field changed | |
| changed_field = get_changed_field_from_update_fields( | |
| update_fields, CHALLENGE_SYNC_FIELDS | |
| ) | |
| # If not from update_fields, try payload keys | |
| if not changed_field: | |
| changed_field = get_changed_field_from_payload( | |
| get_payload_keys(), CHALLENGE_SYNC_FIELDS | |
| ) | |
| if not changed_field: | |
| logger.debug( | |
| f"Could not determine changed field for challenge {challenge.pk}" | |
| ) | |
| return | |
| # Mark as synced to prevent re-entry | |
| mark_synced("Challenge", challenge.pk) | |
| # Serialize and queue the sync task | |
| serialized_challenge = serializers.serialize("json", [challenge]) | |
| github_challenge_sync.delay(serialized_challenge, changed_field) | |
| logger.info( | |
| f"Queued GitHub sync for challenge {challenge.pk}, field: {changed_field}" | |
| ) | |
| # Determine which fields changed | |
| changed_fields = set() | |
| if update_fields: | |
| changed_fields.update( | |
| field for field in update_fields if field in CHALLENGE_SYNC_FIELDS | |
| ) | |
| payload_keys = get_payload_keys() | |
| if payload_keys: | |
| changed_fields.update( | |
| field for field in payload_keys if field in CHALLENGE_SYNC_FIELDS | |
| ) | |
| if not changed_fields: | |
| logger.warning( | |
| f"Could not determine changed syncable fields for challenge {challenge.pk}. " | |
| "Sync to GitHub skipped. update_fields: %s, payload_keys: %s", | |
| update_fields, payload_keys | |
| ) | |
| return | |
| # Mark as synced to prevent re-entry | |
| mark_synced("Challenge", challenge.pk) | |
| # Serialize and queue the sync task for all changed fields | |
| serialized_challenge = serializers.serialize("json", [challenge]) | |
| for changed_field in changed_fields: | |
| github_challenge_sync.delay(serialized_challenge, changed_field) | |
| logger.info( | |
| f"Queued GitHub sync for challenge {challenge.pk}, field: {changed_field}" | |
| ) |
Summary
This PR implements the bi-directional GitHub sync feature as requested in issue Cloud-CV#4760 (originally discussed in PR Cloud-CV#3565).
Currently, EvalAI only supports one-way sync — changes made in the linked GitHub repository are pulled into EvalAI. However, when challenge organizers make changes through the EvalAI web UI (like updating the description, dates, or evaluation details), those changes are not synced back to the GitHub repository. This creates a mismatch between what's in GitHub and what's actually running on EvalAI.
This PR adds the missing reverse sync capability so that changes made via the EvalAI UI are automatically pushed back to the linked GitHub repository.
What's Changed
New Files Added
apps/challenges/github_interface.pyGithubInterface) for communicating with the GitHub APIchallenge_config.yamlwith proper field processingevalai_bot:for easy identificationapps/challenges/github_sync_config.pyapps/challenges/github_utils.pygithub_challenge_sync,github_challenge_phase_syncapps/challenges/migrations/0114_add_github_token_field.pygithub_tokenfield to the Challenge modelModified Files
apps/challenges/models.pygithub_tokenfield to store the GitHub PAT for each challengepost_savesignals for bothChallengeandChallengePhaseapps/challenges/serializers.pygithub_tokento bothChallengeSerializerandZipChallengeSerializerHow It Works
Challenge organizer configures their challenge with:
github_repository(e.g.,cloudcv/evalai-starter)github_branch(e.g.,master)github_token(GitHub Personal Access Token with repo access)When the organizer updates a challenge field through the UI:
post_savesignal firesThe Celery task:
challenge_config.yamlfrom GitHubevalai_bot: Update challenge_config.yaml - changed field: descriptionLoop Prevention
Several safeguards are in place to prevent infinite sync loops:
evalai_bot:for identificationTesting
To test this feature:
reposcopeRelated Issues
Checklist
github_tokenfield with migrationGithubInterfacefor GitHub API communicationpost_savesignals for Challenge and ChallengePhasegithub_token