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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from Dec 30, 2022
+2 −9
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 2 additions & 9 deletions cmd/compose/convert.go
View file
Open in desktop
Expand Up @@ -17,11 +17,9 @@
package compose

import (
"bufio"
"bytes"
"context"
"fmt"
"io"
"os"
"sort"
"strings"
Expand Down Expand Up @@ -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 {
Copy link
Member

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)

Copy link
Contributor Author

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:

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.

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))
Copy link
Member

Choose a reason for hiding this comment

The 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
}

Expand Down