[Enhancement]: support errors from container option

[stevenh]

Proposal

The current version of ContainerCustomizer provides the ability to customise a container in setup but as doesn't return an error so methods which do need to handle errors either have to panic or just the log the error and hope the user checks the log file, neither of which is a good user experience.

I propose we introduce a new type and interface that allows consumers to gracefully upgrade to a flow which will allow handling of errors from container customisation.

For example:

// SafeContainerCustomizer is an interface that can be used to customize a
// testcontainers container request and return an error if the customization
// fails.
type SafeContainerCustomizer interface {
   // SafeCustomize applies the option to the container request and returns an
   // error if the customization fails.
   SafeCustomize(req *GenericContainerRequest) error
}

// SafeContainerOption is a function that modifies a container request.
type SafeContainerOption func(req *GenericContainerRequest) error

// SafeCustomize implements the SafeContainerCustomizer interface.
func (opt SafeContainerOption) SafeCustomize(req *GenericContainerRequest) error {
   return opt(req)
}

// Customize implements the ContainerCustomizer interface.
// It panics if the customization fails.
// Deprecated: Use CustomizeSafe instead.
func (opt SafeContainerOption) Customize(req *GenericContainerRequest) {
   if err := opt(req); err != nil {
   	panic(err)
   }
}

This will allow options which can encounter an error to switch from returning a ContainerOption to SafeContainerOption in a way which is compatible.

Handlers would need to check if the type implements SafeContainerOption and use it in preference for example:

	for _, opt := range options {
		if safeOpt, ok := opt.(testcontainers.SafeContainerCustomizer); ok {
			if err := safeOpt.SafeCustomize(&req); err != nil {
				return nil, err
			}
		} else {
			opt.Customize(&req)
		}
	}

We could change the ContainerCustomizer, given testcontainers is still pre v1,but not a nice user experience, but it should be removed in v2 in my opinion.

Thoughts?

Happy to do implement if folks agree.

[mdelapenya]

I like the idea @stevenh, in fact I've thinking a few times how to deal with that but always without enough bandwidth to do a deep research, thanks for coming with your ideas.

If they community likes it, I support it as a "breaking change" for the current in-development version, but as part of the public API for an eventual v1.

[stevenh]

Thanks @mdelapenya, to confirm you're happy to just make the ContainerCustomizer.Customize return an error, no need to have the compatibility workaround?

[stevenh]

I had an hour, so went ahead with what I assumed from your comment #2267 is now up for review.

Wasn't sure if there was some where else this type of change should be documented, but put it on the options page.

[mdelapenya]

Hey @stevenh I've been thinking about this today more in depth. I think any option should do one of these two things: configure the container request, or add code to be executed honouring the container lifecycle hooks.

The first one, defining the container request, in the worst case scenario, could lead to an inconsistent state, and that should be captured by the RunContainer function because the container won't be able to start. The most flagrant use case would be passing an image that does not exist: WithImage("foo/kjasdasdh:mm").

The second one, leveraging the container lifecycle hooks: they add functions that already return error and are handled by the container lifecycle, so it should cover any kind of validation you want to do in the container for each event (pre/post create/stop/start/ready/stop), signalling the caller about that in the RunContainer function too, and possibly adding custom error messages for it.

Is that the case you want to see that validation support in the functional options for the containers/modules? If that's the case, I think it's responsibility of the caller to pass the right values and not from the library.

I think I have to step back in my previous comment, and at least start the discussion from this point. Sorry if I put you to work before having these thoughts 🙏

[stevenh]

While most options for configuring the request will be error free, there are definitely cases that can and in fact already do trigger an error. These errors need to be returned early as they are preventing the request being configured as requested.

This isn't something that would be covered by the lifecycle management if that's what you were thinking?

[stevenh]

Some examples of failures that are currently hidden or result in a panic are WithNewNetwork and Neo4j, Clickhouse options.

Given Customize is the only way consumers can mutate the request before it's sent when using RunContainer of modules we can't assume that those mutations will be error free as it depends entirely on the use case which we can't predict.

Hope that clarifies why options need to be able to handle errors?

[mdelapenya]

I think we are going to work on this, thanks for bringing the topic to the table. I see it more and more that it needed, so thank you again for convincing me :)

We need a solid plan for communicating the breaking changes: in the docs, in the public APIs, and in the community (probably on Slack)

I'd appreciate your help in this mini-project 😅

[stevenh]

Thanks @mdelapenya happy to help, see you have some comments on the draft PR already.

Where would you like to centralise the conversation about the implementation, should that be here or on a PR?

[mdelapenya]

I think the PR is fine, so we are closer to the code.