OCPBUGS-85713: set WorkingDir from --delete-yaml-file path - #1500
OCPBUGS-85713: set WorkingDir from --delete-yaml-file path#1500shishind wants to merge 1 commit into
Conversation
Derive WorkingDir from the delete YAML location so logs use an absolute writable path when --generate is not used. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@shishind: This pull request references Jira Issue OCPBUGS-85713, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
WalkthroughThe delete CLI now derives ChangesDelete working directory resolution
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: 🟡 Moderate · up to The change can place logs in the wrong directory when a custom delete YAML resides under a directory named delete, so the PR should not merge until generated and custom paths are distinguished. 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ 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 |
openshift-ci-robot
removed
the
jira/invalid-bug
|
@shishind: This pull request references Jira Issue OCPBUGS-85713, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @shishind. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shishind The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@shishind: This pull request references Jira Issue OCPBUGS-85713, which is valid. 3 validation(s) were run on this bug
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. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/pkg/cli/delete.go`:
- Around line 58-62: Update the helper containing the filepath.Base(dir) ==
deleteSubDir check to strip the delete directory only when its parent is the
generated workingDir; preserve the full directory for custom paths such as
/custom/delete/remove.yaml. Add a regression test covering a custom YAML path
inside a delete-named directory and verify setupLogsLevelAndDir uses that
directory for logs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d6a5e711-94b2-417a-8a3d-ea107c0e1e31
📒 Files selected for processing (2)
internal/pkg/cli/delete.gointernal/pkg/cli/delete_test.go
| if filepath.Base(dir) == deleteSubDir { | ||
| return filepath.Dir(dir), nil | ||
| } | ||
| // Custom paths still need an absolute parent for logs. | ||
| return dir, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Distinguish generated paths from custom paths.
Line 58 treats every YAML file in a directory named delete as a generated delete file. For /custom/delete/remove.yaml, the helper returns /custom, but custom-path behavior requires /custom/delete. setupLogsLevelAndDir then creates logs in the wrong directory.
Only strip delete when its parent is the generated workingDir directory. Add a regression test for a custom YAML path in a directory named delete.
Proposed fix
dir := filepath.Dir(absPath)
- if filepath.Base(dir) == deleteSubDir {
- return filepath.Dir(dir), nil
+ parentDir := filepath.Dir(dir)
+ if filepath.Base(dir) == deleteSubDir && filepath.Base(parentDir) == workingDir {
+ return parentDir, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if filepath.Base(dir) == deleteSubDir { | |
| return filepath.Dir(dir), nil | |
| } | |
| // Custom paths still need an absolute parent for logs. | |
| return dir, nil | |
| parentDir := filepath.Dir(dir) | |
| if filepath.Base(dir) == deleteSubDir && filepath.Base(parentDir) == workingDir { | |
| return parentDir, nil | |
| } | |
| // Custom paths still need an absolute parent for logs. | |
| return dir, nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/pkg/cli/delete.go` around lines 58 - 62, Update the helper
containing the filepath.Base(dir) == deleteSubDir check to strip the delete
directory only when its parent is the generated workingDir; preserve the full
directory for custom paths such as /custom/delete/remove.yaml. Add a regression
test covering a custom YAML path inside a delete-named directory and verify
setupLogsLevelAndDir uses that directory for logs.
2 participants
Summary
oc-mirror delete --delete-yaml-file(without--generate), deriveWorkingDirfrom the YAML path (…/working-dir/delete/delete-images*.yaml).mkdir logs: permission deniedwhen the process CWD is not writable (e.g. container with read-only root).CompleteDelete.Fixes https://issues.redhat.com/browse/OCPBUGS-85713
Test plan
CGO_ENABLED=0 go test -tags "json1 exclude_graphdriver_devicemapper exclude_graphdriver_btrfs containers_image_openpgp" ./internal/pkg/cli/ -run 'TestWorkingDirFromDeleteYaml|TestExecutorCompleteDelete|TestExecutorValidateDelete' -v--delete-yaml-file …/working-dir/delete/delete-images.yamlfrom a non-writable CWD and confirm logs land underworking-dir/logsSummary by CodeRabbit
New Features
deletesubdirectories.Bug Fixes