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.

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

refactor: Reuse the the builder configuration in local mode #1

Conversation

HofmeisterAn
Copy link

Hi @nkz-soft 👋 I finally had some time to look into and test the pull request related to the Compose feature. While reviewing and testing the PR, I made some changes that I would like to discuss with you first:

  1. The DockerComposeBuilder.Build() member can determine which DockerComposeContainer implementation should be returned. I don't believe we need DockerCompose and the proxy implementation. Additionally, to 2. this reduces the amount of necessary configurations.
  2. I propose to reuse the builder configuration in the local Docker Compose mode. The command line builder is probably unnecessary as well. The idea is to set the entrypoint configuration to the compose command, such as docker compose --file docker-compose.yml, and use the command configuration for the actual compose command options, e.g., up [OPTIONS]. This has the advantage that developers can easily append options using the WithCommand(string) API.
    • Using this approach makes exposing additional (compose) options, such as RemoveImages, unnecessary as well. We can implement WithRemoveImages(RemoveImages) simply by:
    public DockerComposeBuilder WithCleanUp(RemoveImages removeImages)
    {
        return WithCommand("--rmi", removeImages);
    }

WDYT? Out of simplicity, I removed some parts related to the tests. I would like to discuss a few things first before we finalize the tests again.

[Fact]
[Trait(nameof(DockerCli.DockerPlatform), nameof(DockerCli.DockerPlatform.Linux))]
public void ContainerStartedSuccessfully()
{
// TODO: How do we test that the Compose configuration is actually working? How do we access services?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we actually test if the configuration is running? How do we obtain the endpoint of a service, such as the actual hostname and port?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To fully support tests with services or containers, we need to introduce an additional abstraction, like listChildContainers in Java. At the moment I would limit myself to checking availability using healthchecks in the file itself.

@nkz-soft
Copy link
Owner

nkz-soft commented Apr 7, 2024

Hi @HofmeisterAn.

Unfortunately, I've been very busy for the last month.
Let's agree on which Java implementation features we will support?

The DockerComposeCommandLineBuilder class was made just to be able to handle methods like WithOptions, RemoveImages and others. The main idea was that the options can be applied both at startup (which can be replaced by calling WithCommand) and when the container is stopped (e.g. -rmi). If we don't support these features in this version, there's no question. But I think we will have to come back to it later anyway.

Anyway, this seems like a first step and we can add the necessary functionality later if needed.

@nkz-soft nkz-soft merged commit f1417d4 into nkz-soft:feature/support-docker-compose Apr 19, 2024
@HofmeisterAn HofmeisterAn deleted the nkz-soft-feature/support-docker-compose-1 branch May 4, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants