Skip to content
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

🐛 Close analysis.log before generate static-report #415

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

aufi
Copy link
Member

@aufi aufi commented Jan 9, 2025

Related to containerless --bulk analysis on Windows. Remove of analysis.log silently failed since it was still opened (which appears on Windows only), moving the file Close() from defer to just before generating static-report.

If some error occured before reaching the Close, the file should be closed by golang garbage collector.

Fixes: https://issues.redhat.com/browse/MTA-4307

Related to containerless --bulk analysis on Windows. Remove of
analysis.log silently failed since it was still opened (which appears on
Windows), moving the file `Close()` from `defer` just before generating
static-report.

Fixes: https://issues.redhat.com/browse/MTA-4307

Signed-off-by: Marek Aufart <[email protected]>
@aufi aufi added the cherry-pick/release-0.6 This PR should be cherry-picked to release-0.6 branch label Jan 9, 2025
@aufi
Copy link
Member Author

aufi commented Jan 9, 2025

Succesfuly tested this change on Win10.

@shawn-hurley
Copy link
Contributor

The thing here, is that the defer will close the file on all of the error returns.

from godoc:

Close closes the [File](https://pkg.go.dev/os#File), rendering it unusable for I/O. On files that support [File.SetDeadline](https://pkg.go.dev/os#File.SetDeadline), any pending I/O operations will be canceled and return immediately with an [ErrClosed](https://pkg.go.dev/os#ErrClosed) error. Close will return an error if it has already been called.

I think that right now we are just throwing away that error, so I would just add back the defer. It will error most times but that will be fine because we are not using it.

@aufi
Copy link
Member Author

aufi commented Jan 13, 2025

Thanks for feedback @shawn-hurley! Just to be sure I understand correctly you're suggesting putting back file.Close in defer and keep file.Close also before static-report generation, correct?

@shawn-hurley
Copy link
Contributor

That is correct

Signed-off-by: Marek Aufart <[email protected]>
@aufi aufi merged commit 58bea81 into konveyor:main Jan 23, 2025
3 checks passed
github-actions bot pushed a commit that referenced this pull request Jan 23, 2025
* Close analysis.log before generate static-report

Related to containerless --bulk analysis on Windows. Remove of
analysis.log silently failed since it was still opened (which appears on
Windows), moving the file `Close()` from `defer` just before generating
static-report.

Fixes: https://issues.redhat.com/browse/MTA-4307

Signed-off-by: Marek Aufart <[email protected]>

* Re-add Close in defer

Signed-off-by: Marek Aufart <[email protected]>

---------

Signed-off-by: Marek Aufart <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>
aufi added a commit that referenced this pull request Jan 24, 2025
…419)

🐛 Close analysis.log before generate static-report (#415)

* Close analysis.log before generate static-report

Related to containerless --bulk analysis on Windows. Remove of
analysis.log silently failed since it was still opened (which appears on
Windows), moving the file `Close()` from `defer` just before generating
static-report.

Fixes: https://issues.redhat.com/browse/MTA-4307



* Re-add Close in defer



---------

Signed-off-by: Marek Aufart <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick/release-0.6 This PR should be cherry-picked to release-0.6 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants