Deffiss / testenvironment-docker

MIT License
117 stars 30 forks source link

Review/Improve library API #23

Open Deffiss opened 4 years ago

Hellevar commented 4 years ago

Below you can find my thoughts of configuration options messed up with possible new functionality... What we can configure now:

Which configuration we can introduce:

There are several ways of providing such configuration Builder pattern:

var container = new ContainerBuilder()
    .SetName("postgres")
    .SetPorts(5000, 5001)
    .SetEnvironment(...)
    .Build()

Options way:

var options = new ContainerCreationOptions
{
    Name = "postgres",
    Ports = new Dictionary<int, int> { { 5000, 5001 } },
    Environment = ...
};
var container = new Container(options);
Hellevar commented 4 years ago

Also it might worth to try integrate dockerignore feature based on this library

Deffiss commented 4 years ago

Both configuration options looks good but building nicely looking Fluent API might be difficult. That's why second option looks a bit more realistic. Also would be interesting to see how this might look for specific containers like Mssql or Mongo. In terms of new features, all of them deserve to be introduced but let's prioritize them first and implement as a separate issues.

jzabroski commented 4 years ago

There are several ways of providing such configuration Builder pattern:

var container = new ContainerBuilder()
    .SetName("postgres")
    .SetPorts(5000, 5001)
    .SetEnvironment(...)
    .Build()

How about FluentMigrator-like syntax:

public static class ContainerNames
{
  public static readonly string Postgres = "postgres";
}
public static class EnvironmentNames
{
  public static readonly string Production = "Production";
}
public static class LabelNames
{
  public static readonly string IntegrationTest = "IntegrationTest";
}
var iContainerSyntax = Create.Container(ContainerNames.Postgres)
  .Ports(5000, 5001)
  .Environment(EnvironmentNames.Production)
  .Labels(LabelNames.IntegrationTest)
  .Build(); // builds object

I removed the Set prefix as it interferes with intellisense and minimizing keystrokes to get to an autocompletion result.

May also want to consider @kzu library for hiding certain things from IntelliSense. https://github.com/kzu/IFluentInterface

jzabroski commented 4 years ago

@Deffiss @Hellevar Thoughts on my proposal above?

Another slight tweak would be, instead of .Build(); simply .Do(); I've also used .BuildIt(); in the past.

jzabroski commented 4 years ago

Another thought is that you can have the fluent syntax do something like:

var iContainerSyntax = Create.Container(ContainerNames.Postgres)
  .Ports(5000, 5001)
  .Environment(EnvironmentNames.Production)
  .Labels(LabelNames.IntegrationTest)
  .Entrypoint("RUN.CMD")
  // generates a file named "dockerfile" from the above instructions
  .SaveAsDockerfile(System.IO.Directory.GetCurrentDirectory())
  //.Build() // optional debug step to build the image from the dockerfile
  .Run(); // runs the dockerfile

This would allow you to share dockerfiles with other teams. And since you have a way to specify labels, and also C# compiler metadata functions like CallerMemberName, you can comment the generated dockerfile so people know how you achieved it.

Deffiss commented 4 years ago

@jzabroski Right now we are trying to understand what will be more convenient for usage - fluent notation or just options with properties. Soon we will see some PRs and be able to compare. Your opinion would be appreciated. The idea to expose a dockerfile deserves separate feature thread.

jzabroski commented 4 years ago

@Deffiss I think my core observation is whether .Build() should build an image, or build an object. The more I think about it, it should be Build an image. That's why the dockerfile idea is material. (Although, I think you might be able to dump from the docker image the effective dockerfile, I'm not sure how it works when you construct an image and run it from the API.)