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

Fixes #75 - Cleanup stale file handle #92

Merged
merged 1 commit into from Oct 3, 2017

Conversation

timothysc
Copy link
Member

Fixes #75
Signed-off-by: Timothy St. Clair timothysc@gmail.com

@timothysc timothysc requested a review from chuckha October 2, 2017 21:35
Copy link
Contributor

@chuckha chuckha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one tiny comment

lgtm

@@ -36,6 +36,7 @@ import (
func GatherResults(waitfile string, url string) error {
var inputFileName []byte
var err error
var outfile * os.File
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gofmt this bad boy

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, on save..

"go.formatOnSave": true, 
"go.formatTool": "gofmt",

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weird!! When I save it it moves that * over to be var outfile *os.File

I literally don't care on this issue. Feel free to push.

Copy link

@davecheney davecheney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know much about the history of DoRequest, but it looks like the API makes it hard to manage the lifetime of the io.Reader returned from the anon func.

return outfile, errors.WithStack(err)
err = DoRequest(url, func() (io.Reader, error) {
outfile, e2 := os.Open(s)
return outfile, errors.WithStack(e2)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this; outfile on line 66 is in a different scope to line 39, so on line 70, outfile will always be nil.

I suggest

s/outfile/f here to make it clear that you're talking about a different variable.

@@ -36,6 +36,7 @@ import (
func GatherResults(waitfile string, url string) error {
var inputFileName []byte
var err error

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete this line, use err := on line 65

Signed-off-by: Timothy St. Clair <timothysc@gmail.com>
@timothysc
Copy link
Member Author

Thx for the quick review.

Copy link

@davecheney davecheney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor followups when you have a chance.

@@ -36,6 +36,7 @@ import (
func GatherResults(waitfile string, url string) error {
var inputFileName []byte
var err error
var outfile *os.File

// just loop looking for a file.
done := false

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is out of scope, but what about

for {
// do work
if err == nil {
// success!!
break
}
// log and wait
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patches welcome ;-)
fwiw I didn't write this part of the code, I'm just trying to throw some fixes in before release.

@@ -60,9 +61,16 @@ func GatherResults(waitfile string, url string) error {
url += "." + filenameParts[1]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 45: inputFileName is misleading, it's not the name of a file, it's the contents of waitfile.

@timothysc timothysc merged commit d8711aa into vmware-tanzu:master Oct 3, 2017
@davecheney
Copy link

davecheney commented Oct 3, 2017 via email

jhamilton1 pushed a commit to jhamilton1/sonobuoy that referenced this pull request Jun 27, 2018
Fixes vmware-tanzu#75 - Cleanup stale file handle
Signed-off-by: Jesse Hamilton jesse.hamilton@heptio.com
jhamilton1 pushed a commit to jhamilton1/sonobuoy that referenced this pull request Jun 27, 2018
Fixes vmware-tanzu#75 - Cleanup stale file handle
Signed-off-by: Jesse Hamilton jesse.hamilton@heptio.com

Signed-off-by: Jesse Hamilton jesse.hamilton@heptio.com
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants