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 empty file when using compose config in case of smaller source files #10129
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,11 +17,9 @@ | |
package compose | ||
|
||
import ( | ||
"bufio" | ||
"bytes" | ||
"context" | ||
"fmt" | ||
"io" | ||
"os" | ||
"sort" | ||
"strings" | ||
|
@@ -139,15 +137,10 @@ func runConvert(ctx context.Context, streams api.Streams, backend api.Service, o | |
return nil | ||
} | ||
|
||
var out io.Writer = streams.Out() | ||
if opts.Output != "" && len(content) > 0 { | ||
file, err := os.Create(opts.Output) | ||
if err != nil { | ||
return err | ||
} | ||
out = bufio.NewWriter(file) | ||
return os.WriteFile(opts.Output, content, 0o666) | ||
} | ||
_, err = fmt.Fprint(out, string(content)) | ||
_, err = fmt.Fprint(streams.Out(), string(content)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could even be; _, err = streams.Out().Write(content) But no need to change; there's tons of places where that's not used (so should perhaps be looked at throughout the code) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for sharing this so I can learn from it. I appreciate it. |
||
return err | ||
} | ||
|
||
|
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.
The only thing I'm looking at here (but this was already the case); should
let(content) == 0
be an error condition? In what situation would an empty content be created, and should that be an error?Currently, the code skips creating the file (which is good), but then continues to writing "nothing" to stdout; maybe it should be an error if there's no content created 🤔
(Probably better for maintainers to have a look at, and likely better for a follow-up)
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.
Good point! The code would cleaner, although I don't think the content could be empty without throwing an error before that line. The content of the compose file is validated before that, so even if I have an empty compose file, that would be invalid, therefore it couldn't reach the line of that check. If I have a valid compose file, even if it doesn't have any services only a mapping that could be used for anchors like this:
It would result a longer and not shorter output:
Even if I use
--format json
, the project name and the empty services mapping would be generated.If there is a way we don't know about to have an empty output without error messages, simply just removing
&& len(content) > 0
could be a good way to solve this, so you could have an empty file as you would expect it when you have a valid empty output.Of course other parts of the code could be changed later to eventually generate an empty and valid output, but I agree, this is something that the maintainers should decide how they want to handle it.
Again, thanks for pointing it out :) I enjoy learning about the code of Docker Compose.