issue 285

[goforbroke1006]

https://github.com/testcontainers/testcontainers-go/issues/285 add constructor that takes []io.Reader

[mdelapenya]

Hi @goforbroke1006 , thanks for your first contribution to TC-go.

We are currently reviewing https://github.com/testcontainers/testcontainers-go/pull/463, which could cause this PR to be updated.

If you agree, I'd like to have your PR after the other one.

[goforbroke1006]

Hi @mdelapenya , Ok!

[mdelapenya]

Hi @goforbroke1006 we have merged #476, which adds native support for docker compose. Would you mind revisiting this PR and update to that code? I can assist if needed, or we can even ping @baez90 for that.

Thanks in advance

[goforbroke1006]

Hi @mdelapenya , I take fresh main branch and updated code.

[mdelapenya]

@baez90, as contributor of the compose code, would you mind giving a quick look at this PR? 🙏

[prskr]

Sorry I gave it a quick look but haven't had time to review it properly.

But what came to my mind is: if tc-go should support []io.Reader (or similar) as source I would rather replace the call to ProjectFromOptions(...) with a custom implementation (including custom options that allow to either set file paths or

type NamedReader interface {
    io.Reader
    Name() string
}

(by default implemented e.g. by os.File)

to be able to create a types.Project.

Regarding the option interface we're currently using: it'd be possible to extend the signature to be able to return an error if necessary because the interface should not be implemented outside of tc-go therefore changing the signature shouldn't be a breaking change although I'm not sure if this is necessary considering the alternative to create the NamedReader instance(s) (or a convenience wrapper like func NamedReaderFor(r io.Reader, name string) tc.NamedReader) and use these to create the types.Project instance. What do you think?

Generally I'm wondering what kind of use case you have for this feature 😅 are you generating stack files or downloading them?

[mdelapenya]

Thanks @baez90 for your assistance, I wonder if we could tackle your comments in a follow-up PR of in this one, so that we can see progress on it. What would it be your preference? @goforbroke1006 and yours?

[prskr]

If you'd prefer to split it I'd probably not update the signature of the options interface because ideally we don't need an error return there and a panic(err) until it's refactored would be a fair compromise in the meantime?

IMHO options should not include (much) logic but should only influence logic when they are applied hence the requirement of returning an error indicates a design issue for an option.

I'm still curious what the use case of the whole PR is and if this is actually within the scope of tc-go. As soon as we accept io.Reader instances which we store in temporary files the logical next step would be to also clean them when the types.Project is instantiated. Or is that then out of scope? What about naming of the temporary files because I think docker/compose is using the names for error reporting when parsing the file (s) and so on :blush: Having said that I'm not against adding support for []io.Reader but there are some things to consider to keep everything concise and maintainable.

[kiview]

I would definitely second @baez90 in asking for the context of the PR. Like this, it is unclear to me why it should be added, besides adding a technical capability.

[goforbroke1006]

@baez90 Hi

1. Panic - I decide use this way notify user about system problems (file/dir permission etc). Better handle errors, but interface design does not allow it.
2. Logic in options - I can try to move logic to some helper-function to keep options simple. I have no experience in this kind of question. Hope you give advise.
3. Purpose of PR - if I remember right it was proposal add logic to run tests with Stdin as source for docker-compose. I decide io.Reader better solution to satisfy this request.

[prskr]

1. Probably I did not make myself clear: I understand why you're panic()-ing here, but the interface design is like this for a reason :wink: options should not need to handle errors
2. it is not about whether you put the logic directly in the options or move it to a helper function but about how options are generally working. In most cases they encapsulate values that replace default values of some 'arguments struct' that is then used e.g. for initializing. Consequently you should not read the []io.Reader instances in the option but extend the composeStackOptions to either use []io.Reader or []string as stack file source in the NewDockerCompose() function (where you could return an error already)
3. I'm not 100% sure if the original request from @normanjaeckel still makes sense with the new compose API. Probably an option to pass an already initialized types.Project would make more sense and would be more flexible?

[goforbroke1006]

Guys, I see. About light-weight options - I agree. And maybe this feature with reader is not actual now. So, you can just reject PR ) Or I can revert all changes and just create example of code with DockerCompose and readers.

[mdelapenya]

maybe this feature with reader is not actual now. So, you can just reject PR Or I can revert all changes and just create example of code with DockerCompose and readers.

@goforbroke1006 sorry but I missed your comment as I went on parental leave on Dec 10th.

If you agree, let's close this PR so you could work on a brand new PR with the compose examples. Thanks for your time!