Conversation
There was a problem hiding this comment.
This seems to be an extra or unsaved file.
| public string Repository { get; set; } | ||
| public bool InitSubmodules { get; set; } | ||
| public string LocalFolder { get; set; } | ||
| public string Archive { get; set; } |
There was a problem hiding this comment.
Pull Request Overview
This PR adds archive source option functionality to Crank, allowing users to upload ZIP files or have the agent pull them from URLs as an alternative to git-based source code delivery. This enables benchmark setup without requiring a git client.
Key changes:
- Added
Archiveproperty to source models to support ZIP archive sources - Implemented archive handling logic in both controller and agent components
- Added validation to ensure each source has at least one code location defined
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| hello-archive.benchmarks.yml | Test configuration demonstrating archive source usage |
| CommonTests.cs | Integration test for archive functionality |
| Source.cs | Added Archive property to source models |
| Job.cs | Updated job source mapping to include Archive property |
| README.md | Updated documentation with archive source option |
| Program.cs | Added source validation and archive path resolution logic |
| JobConnection.cs | Added local archive upload functionality |
| Documentation.cs | Updated help documentation for archive option |
| Startup.cs | Added archive extraction logic in the agent |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| if (archivePath.StartsWith("http", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| var tempArchive = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName() + ".zip"); |
There was a problem hiding this comment.
Using Path.GetRandomFileName() for temporary files can create predictable filenames. Consider using Path.GetTempFileName() which creates a unique temporary file with proper permissions, or use a cryptographically secure random generator for the filename.
| var tempArchive = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName() + ".zip"); | |
| // Use Path.GetTempFileName() to securely create a unique temp file, then rename to .zip | |
| var tempFile = Path.GetTempFileName(); | |
| var tempArchive = Path.ChangeExtension(tempFile, ".zip"); | |
| File.Move(tempFile, tempArchive); |
|
|
||
| ZipFile.ExtractToDirectory(tempArchive, targetDir); | ||
|
|
||
| try { File.Delete(tempArchive); } catch { } |
There was a problem hiding this comment.
Silent exception swallowing in cleanup code can hide important errors. Consider logging the exception or at minimum catching specific exceptions like IOException or UnauthorizedAccessException that are expected during file cleanup.
| try { File.Delete(tempArchive); } catch { } | |
| try | |
| { | |
| File.Delete(tempArchive); | |
| } | |
| catch (IOException ex) | |
| { | |
| Log.Warning(ex, $"Failed to delete temporary archive file '{tempArchive}' due to IO error."); | |
| } | |
| catch (UnauthorizedAccessException ex) | |
| { | |
| Log.Warning(ex, $"Failed to delete temporary archive file '{tempArchive}' due to insufficient permissions."); | |
| } |
| } | ||
| finally | ||
| { | ||
| try { File.Delete(helloZip); } catch { } |
There was a problem hiding this comment.
Silent exception swallowing in cleanup code can hide important errors. Consider logging the exception or at minimum catching specific exceptions like IOException or UnauthorizedAccessException that are expected during file cleanup.
| try { File.Delete(helloZip); } catch { } | |
| try | |
| { | |
| File.Delete(helloZip); | |
| } | |
| catch (Exception ex) | |
| { | |
| _output.WriteLine($"[CLEANUP] Failed to delete {helloZip}: {ex.GetType().Name}: {ex.Message}"); | |
| } |
2 participants
Ability to upload a ZIP file or have the agent pull one from a URL. This allows setup without the git client.