Fix empty file when using compose config in case of smaller source files

[rimelek]

"docker compose config --output out.yml" resulted an empty file when the generated output was smaller than 4097 bytes. bufio.Writer doesn't seem necessary since only one write operation will happen. This way there is no need for a new bufio.Writer that could be flushed.

What I did

First I replaced bufio.NewWriter(file) with io.Writer(file), then based on suggestion of @thaJeztah I replaced the whole conditional file writing with a simple io.WriteFile. See the related issue for my previous ideas.

Related issue
closes #10121

[thaJeztah]

Isn't file already a io.Writer?

Wondering if we're missing a defer file.Close()

[thaJeztah]

I guess since it's all writing at once, could also be os.WriteFile(opts.Output, content, 0o666) if a file is specified instead of stdout, which would take care of both creating, truncating, and writing to the file

[rimelek]

@thaJeztah The default output is streams.Out() and the changed line saved a bufio.Writer into out declared as io.Writer. out didn't have a method called Close, but I also tried to close the file. It didn't help. bufio.Writer needs to be flushed unless the output is larger than the buffer size. Closing the file wouldn't have solved this, since the content was not passed to the underlying writer. In this case, the file. Since the end of the runConvert method is using fmt.Fprint to write the content to a file or to stdout depending on the value of out, I thought it was the easiest way to fix it, but you are right. I tried with this much shorter version to handle both stdout and the output file and worked:

	if opts.Output != "" && len(content) > 0 {
		return os.WriteFile(opts.Output, content, 0o666)
	}
	_, err = fmt.Fprint(streams.Out(), string(content))

Should I force push the above code to the source branch?

Edit: I did that

[thaJeztah]

Thanks! Yes, that looks like the correct approach to me (but I'm not a maintainer on this repository, so it's possible that there's perhaps something I'm overlooking 🙈 😅)

[thaJeztah]

LGTM (non-binding, as I'm not a maintainer)

[thaJeztah]

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)

[rimelek]

Thank you for sharing this so I can learn from it. I appreciate it.

[codecov]

Codecov Report

Base: 73.89% // Head: 73.89% // No change to project coverage 👍

Coverage data is based on head (dea484d) compared to base (1d9657a).
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##               v2   #10129   +/-   ##
=======================================
  Coverage   73.89%   73.89%           
=======================================
  Files           2        2           
  Lines         272      272           
=======================================
  Hits          201      201           
  Misses         60       60           
  Partials       11       11           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[thaJeztah]

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)

[rimelek]

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:

x-test:
  hello: world

It would result a longer and not shorter output:

name: phptest
services: {}
x-test:
  hello: world

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.