chore!: export DockerCompose type in compose package

[youshibin]

What does this PR do?

Exports the DockerCompose type in the compose packge.

BREAKING CHANGE: The DockerCompose interface, used for the LocalDockerCompose implementation (shelling out to the local binary), was renamed to DockerComposer to prevent a
naming collision when exporting DockerCompose type.

Why is it important?

Returning an un-exported type from a constructor function limits the flexibility and usability of the returned instance. Callers can use the exported methods, but cannot declare variables of that type, pass instances to other functions, or embed them in structs without exposing the constructor itself.

Related issues

• Closes Export dockerCompose type so consumers can store it in a variable and call Down #2907

[netlify]

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	da1ab41
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/67d81a6ac0ae200008778bd7
😎 Deploy Preview	https://deploy-preview-2953--testcontainers-go.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[stevenh]

Could you clarify why this doesn't work already, as a quick test seems to indicate it does?

func TestHelloWorld(t *testing.T) {
	compose, err := compose.NewDockerCompose("test")
	require.NoError(t, err)
	err = compose.Down(context.Background())
	require.NoError(t, err)
}

[youshibin]

@stevenh Yep, you are absolutely right! The type doesn't need to be exported for the methods which are exported to be accessible outside of the package.

[stevenh]

@stevenh Yep, you are absolutely right! The type doesn't need to be exported for the methods which are exported to be accessible outside of the package.

So we can close this and the associated issue?

[xbc5]

I think the documentation should clarify or emphasise the necessity to use the ComposeStack interface -- it isn't initially obvious that these are related.

[youshibin]

I think the documentation should clarify or emphasise the necessity to use the ComposeStack interface -- it isn't initially obvious that these are related.

@xbc5 I agree, it's a bit confusing, but I believe the reason for the different interface name (ComposeStack) is because the previous interface (DockerComposer) is being deprecated. So, seems like there was a naming conflict.

Where would you like to see documentation updated?

[xbc5]

I think the documentation should clarify or emphasise the necessity to use the ComposeStack interface -- it isn't initially obvious that these are related.

@xbc5 I agree, it's a bit confusing, but I believe the reason for the different interface name (ComposeStack) is because the previous interface (DockerComposer) is being deprecated. So, seems like there was a naming conflict.

Where would you like to see documentation updated?

Inline, on the NewDockerCompose factory; but if it returns the interface then it's not really necessary. It's not my place to say what this function should or should not return, but if it returns a struct, it should mention the interface.

Also, this is the first place I looked to get acquainted with the factory: so perhaps there too? Again, not necessary if it returns the interface.

Thanks for doing this.

[stevenh]

Sorry I think you miss-understood my comment in the issue, I meant we should update the reason for the fix of exporting the returned type to detail that returning an unexported type restricts it's use, as it means it cannot be stored in a type or passed as a variable to a function, for example neither of the following are possible

type MyType struct {
   Stack *dockerCompose
}

func MyFunc(stack *dockerCompose) {
....
}

You should always return types and accept interfaces. For info on why see this.

[youshibin]

@stevenh appreciate the review and sharing your wisdom! Great reference you linked there with returning types and accepting interfaces!

I went back to the original implementation, which exports the DockerCompose type in the constructor functions. Due to the naming conflict with the (deprecated) interface, that had to be renamed.

There was a cleanup helper method which used to accept a *dockerCompose, but now accepts a ComposeStack interface.

I don't suppose adding an interface implementation check (or interface compliance assertion) would be useful here to drive home the fact that the DockerCompose type implements the ComposeStack interface?

var _ ComposeStack = (*DockerCompose)(nil)

[stevenh]

One clarifying question.

Due to the breaking change the PR title also needs a ! adding to it and "BREAKING CHANGE" to the description see conventional commits.

[mdelapenya]

LGTM, thanks @jasonyunicorn for your work here 🙇

Waiting for a final +1 from @stevenh to merge this.

[stevenh]

LGTM