-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(subcmds/saas): remove timestamped directory in results-dir #2248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(subcmds/saas): remove timestamped directory in results-dir #2248
Conversation
Thank you very much for contributing!! Before detailed review, please let me point two things:
Thanks again! |
7a91979
to
682f471
Compare
23b6e4a
to
c3e2a94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ensures the results directory is purged after every upload attempt (success or failure) by moving the cleanup logic into a deferred function and removing the original post-success cleanup block.
- Moved
os.RemoveAll(dir)
into adefer
so it always runs on exit - Retained the debug flag check to skip removal in debug mode
- Removed the redundant cleanup block at the end of the function
Comments suppressed due to low confidence (2)
subcmds/saas.go:85
- [nitpick] The local variable
dir
is generic and could be renamed toresultsDir
(or similar) to clarify its purpose in cleanup.
defer func() {
subcmds/saas.go:85
- Consider adding unit tests that simulate both success and failure paths to verify the results directory is removed in all cases.
defer func() {
Co-authored-by: Shunichi Shinohara <[email protected]>
c3e2a94
to
e68b0f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an error occurs during the next scan and the timestamped directory is not created, there's a possibility that the previous one might be uploaded instead — is that acceptable?
We acknowledge that possibility. We have decided to cover this case with our operational workflow, so this is acceptable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great Job!
Works as advertised!!! 🍻
If this Pull Request is work in progress, Add a prefix of “[WIP]” in the title.
What did you implement:
WHY
WHAT
Type of change
How Has This Been Tested?
Before
After
Checklist:
You don't have to satisfy all of the following.
make fmt
make test
Is this ready for review?: YES
Reference