BED-6446: Add winres tool to generate Windows Resources#169
Open
StranDutton wants to merge 3 commits intomainfrom
Open
BED-6446: Add winres tool to generate Windows Resources#169StranDutton wants to merge 3 commits intomainfrom
StranDutton wants to merge 3 commits intomainfrom
Conversation
WalkthroughAdds a Windows resource generation utility and invokes it from Build and Publish GitHub workflow jobs on the Windows matrix, plus supporting tests, a new Company constant, and dependency/tooling updates in go.mod. Changes
Sequence DiagramsequenceDiagram
participant GH as GitHub Actions
participant CLI as generate-windows-resources
participant FS as File System
participant Tool as go-winres
GH->>CLI: run CLI with version arg (Windows matrix only)
CLI->>CLI: parse/validate version
CLI->>CLI: build winres.json (icons, version, localized metadata)
CLI->>FS: write winres/winres.json
FS-->>CLI: file written
CLI->>Tool: execute "go tool go-winres make"
Tool->>FS: generate resource files (.syso/.res)
FS-->>Tool: resources created
Tool-->>CLI: return exit status
CLI-->>GH: emit success/failure message (allowed to continue on error)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (3)
cmd/generate-windows-resources/generate-windows-resources.go (2)
67-76:parseProductVersionreads from the globalos.Argsdirectly.This works, but makes the function harder to test in isolation without mutating global state (which the tests do). Consider accepting
[]stringas a parameter for better testability. That said, it's a small build utility so this is purely a style suggestion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/generate-windows-resources/generate-windows-resources.go` around lines 67 - 76, Change parseProductVersion to accept an argument slice (e.g., args []string) instead of reading os.Args directly so it becomes testable; update its signature (parseProductVersion(args []string) (string, error)), replace uses of os.Args within the function with the provided args slice (use args[0] for the program name and args[1] for the version), and update any call sites (main or tests) to pass os.Args or a custom slice when testing.
111-130: Deferredf.Close()error is silently discarded.On buffered/networked filesystems,
Closecan be the call that actually flushes and reports a write error. SinceEncodesucceeds doesn't guarantee the data hit disk. Consider checking the close error explicitly:Suggested fix
func writeWinresConfig(config map[string]interface{}) error { if err := os.MkdirAll(winresDir, 0755); err != nil { return fmt.Errorf("failed to create winres directory: %w", err) } configPath := filepath.Join(winresDir, winresJSONFile) f, err := os.Create(configPath) if err != nil { return fmt.Errorf("failed to create %s: %w", configPath, err) } - defer f.Close() enc := json.NewEncoder(f) enc.SetIndent("", " ") if err := enc.Encode(config); err != nil { + f.Close() return fmt.Errorf("failed to encode JSON: %w", err) } - return nil + return f.Close() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/generate-windows-resources/generate-windows-resources.go` around lines 111 - 130, The deferred file Close in writeWinresConfig currently discards any error from f.Close; capture and handle the Close error so flush/write failures aren't ignored—update writeWinresConfig to check the error returned by f.Close (in the defer or after encoder.Encode) and return or wrap that error (e.g., if enc.Encode succeeded but f.Close returns non-nil, return fmt.Errorf("failed to close %s: %w", configPath, err)). Ensure you reference the same file handle (f) and configPath and propagate the Close error instead of silently dropping it..github/workflows/publish.yml (1)
35-40: Double error suppression:continue-on-error: trueand|| echoare redundant.The
continue-on-error: trueon line 37 already ensures the job won't fail if this step errors. The|| echofallback on line 40 provides a friendlier log message but is technically unnecessary for preventing failure. Not a bug—just noting the overlap. Consider keeping only one mechanism for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish.yml around lines 35 - 40, The step named "Generate Windows Resources" uses both continue-on-error: true and a shell fallback "|| echo 'Failed to generate Windows Resources. Skipping...'", which is redundant; edit that step to remove one mechanism (preferably keep continue-on-error: true and delete the "|| echo ..." suffix in the run block) so error suppression is handled in one place and logs remain clean; look for the step name "Generate Windows Resources" and the run command that invokes go run cmd/generate-windows-resources/generate-windows-resources.go to make the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 54: The go.mod currently pulls golang.org/x/image at an old vulnerable
revision; add an explicit module override to bump it to v0.18.0 or later
(recommended v0.36.0) to mitigate CVE-2024-24792 and ensure the indirect
consumer (go-winres) uses the fixed version; update go.mod by adding a
require/replace for golang.org/x/image with the chosen safe version and then run
go mod tidy to update the dependency graph and lockfile.
---
Nitpick comments:
In @.github/workflows/publish.yml:
- Around line 35-40: The step named "Generate Windows Resources" uses both
continue-on-error: true and a shell fallback "|| echo 'Failed to generate
Windows Resources. Skipping...'", which is redundant; edit that step to remove
one mechanism (preferably keep continue-on-error: true and delete the "|| echo
..." suffix in the run block) so error suppression is handled in one place and
logs remain clean; look for the step name "Generate Windows Resources" and the
run command that invokes go run
cmd/generate-windows-resources/generate-windows-resources.go to make the change.
In `@cmd/generate-windows-resources/generate-windows-resources.go`:
- Around line 67-76: Change parseProductVersion to accept an argument slice
(e.g., args []string) instead of reading os.Args directly so it becomes
testable; update its signature (parseProductVersion(args []string) (string,
error)), replace uses of os.Args within the function with the provided args
slice (use args[0] for the program name and args[1] for the version), and update
any call sites (main or tests) to pass os.Args or a custom slice when testing.
- Around line 111-130: The deferred file Close in writeWinresConfig currently
discards any error from f.Close; capture and handle the Close error so
flush/write failures aren't ignored—update writeWinresConfig to check the error
returned by f.Close (in the defer or after encoder.Encode) and return or wrap
that error (e.g., if enc.Encode succeeded but f.Close returns non-nil, return
fmt.Errorf("failed to close %s: %w", configPath, err)). Ensure you reference the
same file handle (f) and configPath and propagate the Close error instead of
silently dropping it.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumwinres/favicon.icois excluded by!**/*.ico
📒 Files selected for processing (6)
.github/workflows/build.yml.github/workflows/publish.ymlcmd/generate-windows-resources/generate-windows-resources.gocmd/generate-windows-resources/generate-windows-resources_test.goconstants/misc.gogo.mod
StranDutton
changed the title
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Ticket
https://specterops.atlassian.net/browse/BED-6446
Summary
go-winresin order to apply manifest information to the.exefor Windows users.buildandpublishGitHub actions in order to generate these resources just before the application is built and published.go-winres.Go 1.23.0 -> 1.24.0- I reran unit tests and performed a smoke test to make sure everything still works as expected after updating.Testing
act(https://github.com/nektos/act?tab=readme-ov-file)Testing Instructions
Goal: run the script I created to generate the Windows resources, build the image locally, and then check the
.exeto make sure that file properties get populated.Install
go-winresonto the windows machine (not in the AzH project directory)go install http://github.com/tc-hib/go-winres@latestAfter
go-winresis installed, pull my branchBED-6446-include-version-in-exe-propsThen run
go run cmd/generate-windows-resources/generate-windows-resources.go "v0.0.0-rolling+8675".sysofile is generatedBuild AzureHound (on Windows) with this command:
Find the
.exeand inspect the properties to ensure everything looks correct.Demo
All information populated as expected when examining the properties of the .exe ✅
Summary by CodeRabbit
New Features
Chores