P1: Include zip in SHA256SUMS.txt for releases #48
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the Windows artifact staging script so that the generated distribution .zip is created before checksum generation and is included alongside .exe files in SHA256SUMS.txt. Sequence diagram for updated checksum generation including zip artifactsequenceDiagram
participant Script as stage_artifacts_ps1
participant FS as FileSystem
participant Archiver as Compress_Archive
participant Hasher as Get_Sha256Hash
Script->>FS: Copy cloudsqlctl.exe to artifacts directory
Script->>FS: Resolve files for zip (exe, installer, README, LICENSE, docs, changelog)
Script->>FS: Filter nonexisting files
Script->>Archiver: Compress files into cloudsqlctl-windows-x64.zip
Archiver->>FS: Write cloudsqlctl-windows-x64.zip
Script->>FS: Get items in artifacts directory
Script->>FS: Filter artifacts by extension (.exe, .zip)
loop For each artifact (.exe and .zip)
Script->>Hasher: Get_Sha256Hash(artifact_path)
Hasher->>FS: Read artifact file
Hasher-->>Script: SHA256 hash
Script->>FS: Append hash and filename to SHA256SUMS.txt
end
Script->>FS: List staged artifacts
Script-->>Script: Stage complete
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- When collecting artifacts to hash, consider adding
-FiletoGet-ChildItemto ensure directories or other non-file entries in$ArtifactsDirare not accidentally included in the checksum list. - You might want to guard
Compress-Archivewith a check that$FilesToZipis non-empty (and/or add-ErrorAction Stop) so the script fails fast with a clear message if expected inputs are missing rather than silently creating an unexpected archive.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When collecting artifacts to hash, consider adding `-File` to `Get-ChildItem` to ensure directories or other non-file entries in `$ArtifactsDir` are not accidentally included in the checksum list.
- You might want to guard `Compress-Archive` with a check that `$FilesToZip` is non-empty (and/or add `-ErrorAction Stop`) so the script fails fast with a clear message if expected inputs are missing rather than silently creating an unexpected archive.
## Individual Comments
### Comment 1
<location> `tools/stage-artifacts.ps1:65` </location>
<code_context>
}
-Get-ChildItem $ArtifactsDir -Filter "*.exe" | ForEach-Object {
+$ArtifactsToHash = Get-ChildItem $ArtifactsDir | Where-Object { $_.Extension -in @('.exe', '.zip') } | Sort-Object Name
+$ArtifactsToHash | ForEach-Object {
$hash = Get-Sha256Hash -FilePath $_.FullName
</code_context>
<issue_to_address>
**suggestion:** Consider restricting `Get-ChildItem` to files only to avoid unexpected directory matches.
Using `-File` makes it clear we only intend to hash files and avoids rare cases where non-file items with matching extensions are returned. For example:
```powershell
$ArtifactsToHash = Get-ChildItem $ArtifactsDir -File |
Where-Object { $_.Extension -in @('.exe', '.zip') } |
Sort-Object Name
```
```suggestion
$ArtifactsToHash = Get-ChildItem $ArtifactsDir -File | Where-Object { $_.Extension -in @('.exe', '.zip') } | Sort-Object Name
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR modifies the artifact staging process to include the Windows distribution zip file in the SHA256SUMS.txt checksum file for releases.
- Reorders the zip file creation to occur before checksum generation (previously after)
- Updates checksum generation to include
.zipfiles in addition to.exefiles - Maintains deterministic output by sorting artifacts by name
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #47
Summary
Local tests
Rollback
Summary by Sourcery
Include the Windows distribution zip in the staged artifacts and checksum generation for releases.
New Features:
Enhancements: