NO-ISSUE: pkg/cli/admin/release/extract: Read manifests into memory#2232
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@wking: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughExtraction now runs in two phases: tar entries are parsed and their contents collected into in-memory records (manifests or raw bytes). After archive parsing completes, a single post-processing pass applies filtering and writes serialized manifests or raw files to disk. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cli/admin/release/extract.go`:
- Around line 489-498: The loop over o.files opens files and uses defer
file.Close() which delays closing until the enclosing function returns, risking
descriptor exhaustion; change this so each file is closed immediately by either
moving the per-file logic into a small helper function (e.g.,
extractOneFile(fileEntry) that creates, writes and defers Close inside it) or by
explicitly calling file.Close() at the end of the loop iteration instead of
deferring; update the code paths that set out (the block using os.Create, file,
err, out and related write logic) to ensure the file handle is closed before the
next iteration.
- Around line 507-548: This block uses out-of-scope symbols and wrong return
types: pull the helper variables and functions (include,
expectedProviderSpecKind) out of the inner if so they are visible here (or move
this whole filtering block back inside the o.ExtractManifests/TarEntryCallback
scope), replace every use of ms with f.manifests and update the exclusion logic
to modify f.manifests in-place (or re-slice into a new slice and assign back to
f.manifests), and change the erroneous return of "return false, fmt.Errorf(...)"
to "return fmt.Errorf(...)" to match Run()'s signature; target symbols to edit
are include, expectedProviderSpecKind, f.manifests and the error return in
Run()/this function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 466311d5-6a73-4e3d-a345-b85844a99e03
📒 Files selected for processing (1)
pkg/cli/admin/release/extract.go
wking
force-pushed
the
release-extract-read-manifests-into-memory-write-after-tar
branch
2 times, most recently
from
5c02456 to
525fd35
Compare
And write them to disk after exiting the tar callback. This uses a
bit more memory during processing, but sets us up for more efficient
two-stage processing (e.g. if we need to extract feature gates from
the manifests to know which manifests need including).
The:
for _, f := range o.file {
err := func() error {
...
defer file.Close()
...
}()
if err != nil {
return err
}
}
closure scopes the defer to one round of the loop [1]:
A "defer" statement invokes a function whose execution is deferred
to the moment the surrounding function returns...
otherwise open file descriptors would accumulate and only get closed
once the whole loop completes. CodeRabbit recommended an explicit
Close, but I prefer the simplicity of defer, even if it means another
level of intentation nesting.
[1]: https://go.dev/ref/spec#Defer_statements
[2]: openshift#2232 (comment)
And write them to disk after exiting the tar callback. This uses a
bit more memory during processing, but sets us up for more efficient
two-stage processing (e.g. if we need to extract feature gates from
the manifests to know which manifests need including).
The:
for _, f := range o.file {
err := func() error {
...
defer file.Close()
...
}()
if err != nil {
return err
}
}
closure scopes the defer to one round of the loop [1]:
A "defer" statement invokes a function whose execution is deferred
to the moment the surrounding function returns...
otherwise open file descriptors would accumulate and only get closed
once the whole loop completes. CodeRabbit recommended an explicit
Close, but I prefer the simplicity of defer, even if it means another
level of intentation nesting.
[1]: https://go.dev/ref/spec#Defer_statements
[2]: openshift#2232 (comment)
wking
force-pushed
the
release-extract-read-manifests-into-memory-write-after-tar
branch
from
525fd35 to
760045b
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/verified by @wking |
openshift-ci-robot
added
the
verified
|
@wking: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
|
@wking: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
And write them to disk after exiting the tar callback. This uses a bit more memory during processing, but sets us up for more efficient two-stage processing (e.g. if we need to extract feature gates from the manifests to know which manifests need including, #2222).