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

Builder Does Not Support Specifying tcp/Arbitrary IHostService, *Builder classes Cannot Be Extended #185

Closed
lordmilko opened this issue Mar 28, 2021 · 9 comments
Assignees
Milestone

Comments

@lordmilko
Copy link

In my program, users will be able to customize the URI of the docker service they wish to connect to via a config file, thereby allowing users to connect to tcp://<arbitraryAddress>:2375 without need to specify this as their DOCKER_HOST environment variable.

At the lowest level, I can easily create an DockerHostService that encapsulates this custom Docker URI, however I've found when it comes to using the fluent API, there doesn't appear to be a way to specify either the arbitrary IHostService I've created, or simply specify a tcp:// based one.

The HostBuilder returned from new Builder().UseHost() only appears to support building three types of IHostService objects

  1. Native, by specifying .UseNative().Build()
  2. An SSH connection, by specifying .UseSsh(...).Build()
  3. A Docker Machine connection, by specifying .UseMachine().Build()

I would propose that an additional method should be added, perhaps UseTcp(...), UseUri(...) or UseHost(IHostService service) that enables an arbitrary host be used in fluent contexts. Users could theoretically implement such methods themselves if they wanted to by extending HostBuilder, however unfortunately it appears that most of the *Builder classes are actually sealed with internal constructors, making this a bit more complicated.

Fortunately, since BaseBuilder<T> simply has a protected constructor I was able to effectively workaround this in my application by creating an IBuilder extension method that returns a custom BaseBuilder<IHostService>, however since I need to re-implement UseContainer this required the use of reflection since the ContainerBuilder constructor is not public.

static class BuilderExtensions
{
    public static HostBuilderEx UseHost(this IBuilder parent, IHostService hostService) =>
        new HostBuilderEx(parent, hostService);
}

class HostBuilderEx : BaseBuilder<IHostService>
{
    private IHostService hostService;

    public HostBuilderEx(IBuilder parent, IHostService hostService) : base(parent)
    {
        this.hostService = hostService;
    }

    public override IHostService Build() =>
        hostService;

    protected override IBuilder InternalCreate() =>
        new HostBuilderEx(this, hostService);

    public ContainerBuilder UseContainer()
    {
        var builder = (ContainerBuilder) Activator.CreateInstance(
            typeof(ContainerBuilder),
            BindingFlags.Instance | BindingFlags.NonPublic,
            null,
            new[] {this},
            null
        );
        Childs.Add(builder);
        return builder;
    }
}
var hostService = new DockerHostService("native", true, false, Settings.DockerUri, isWindowsHost: true);

var container = new Builder()
    .UseHost(hostService)
    .UseContainer()
    ...

As such, I would present it could potentially be useful to implement additional methods on HostBuilder for utilizing additional IHostService types in fluent contexts, and/or unsealing the various *Builder types that are available so that they can be more easily extended.

@mariotoffia mariotoffia self-assigned this Mar 28, 2021
@mariotoffia mariotoffia added this to the 4.0.0 milestone Mar 28, 2021
@mariotoffia
Copy link
Owner

@lordmilko Thanks for this really good explained issue. I'll add support for custom IHostService. I think it's better that we do as you just did - file a issue and I'll or the one files a issue - files a PR so we can make this library great for everyone :).

@mariotoffia
Copy link
Owner

mariotoffia commented Mar 28, 2021

@lordmilko I've added both ability to add a IHostService´ and a shorthand FromUri` so your above example would look like.

using(var container = Fd.UseHost().FromUri(Settings.DockerUri, isWindowsHost: true).
                                  .UseContainer().Build()) 
{
}

There are some other parameters for FromUri. But if that is not sufficient, you may use UseHost(IHostService).

Cheers,
Mario :)

@lordmilko
Copy link
Author

Thanks @mariotoffia,

I also stumbled upon the fact this also appears to be the same issue as described in #105, so potentially this may allow both issues to be closed

@mariotoffia
Copy link
Owner

@lordmilko I would say it partially solves #105 since it was derived another issue from that one - issue #160 so it will support docker-compose along with docker contexts.

I would like to state that I really like your issue, good explanation so it is crystal clear what you want! - Thanks! :)

@lordmilko
Copy link
Author

Hi @mariotoffia,

Today I noticed you had pushed FluentDocker 2.10.2 with this change last week! I was able to try it out and can confirm everything is working as expected; one curious thing I did note however, I personally found the syntax surrounding the new UseHost method a bit confusing.

In the README it states that

UseHost that takes a instantiated IHostService implementation.

While it doesn't provide an example, it seems fairly logical based on this that you can probably do something like:

var hostService = new DockerHostService(...);

var container = Fd
    .UseHost(hostService)
    ...

But this is incorrect! In fact, the syntax is actually

var hostService = new DockerHostService(...);

var container = Fd
    .UseHost()
    .UseHost(hostService)
    ...

In order to use your custom IHostService you have to call two methods called UseHost, where each method has a slightly different meaning (the first refers to using a host Builder, while the second method refers to using a host Service) which strikes me as a bit of a strange design pattern

I think it would probably make more sense as

var container = Fd
    .UseHost()
    .WithHostService(hostService)
    ...

or something, which would be consistent with how you can do

    .UseImage(...)
    .WithName(...)
    .WithHostName(...)

of course, for me my program is now working without the need for my own custom workaround. Potentially this may be something worth considering however.

Thanks for your help!

Regards,
lordmilko

@mariotoffia
Copy link
Owner

@lordmilko Great suggestion!! - Hmm... yes, kid'a cookie ontop of cookie as we say in sweden :). What do you think about

var container = Fd
    .UseHost()
    .WithCustomHost(hostService)
    // or
var container = Fd
    .UseHost()
    .FromService(hostService)
   // or
var container = Fd
    .UseHost()
    .AsService(hostService)

?

Cheers,
Mario :)

@lordmilko
Copy link
Author

lordmilko commented Apr 4, 2021

Hi @mariotoffia,

I spent a bit of time thinking about this! And I would say WithService is probably best

  • WithCustomHost is ambiguous - what is a "host" - are we talking about a host builder or host service. It's also a bit verbose (having only two words in a fluent operation is probably better than three)
  • FromService seems OK at first, however when you compare it to FromUri it doesn't make sense; FromUri creates a host service from a URI for your HostBuilder. FromService doesn't create anything from a service, rather it modifies the HostBuilder to execute with your specified IHostService
  • "as" implies performing a conversion in C#, but AsService doesn't convert anything at all

As such, by process of elimination I think WithService works best

@mariotoffia
Copy link
Owner

Neat! :) I'll add it to my todo for the week - I will make this change in hope that not too many have used this particular method since I'm going to do a rename, hence incompatibility is introduced even if the semver do not denote such - hope this is ok for you.

Thanks for your help!

Cheers,
Mario :)

@mariotoffia
Copy link
Owner

done, will come in a release this week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants