chore!: extract image-related code to the top-level image package#2995
chore!: extract image-related code to the top-level image package#2995mdelapenya wants to merge 6 commits intotestcontainers:mainfrom
Conversation
mdelapenya
added
chore
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
stevenh
left a comment
stevenh
left a comment
There was a problem hiding this comment.
I like the idea of a smaller package with more focused responsibilities.
I've done a quick pass to capture some observations, however I wonder for if the current pattern of passing in a type which implements an interface hides the important elements making it harder to understand the logic and creating a hard dependency on external types such as types.ImageBuildOptions.
Thinking from first principles for a minute, would it be better if we made the image follow the our new best practices. For example have Build method take the required arguments + functional options for example:
func Build(ctx context.Context, context io.Reader, options ... Option) (string, error) {
...
}That way we can avoid hard dependences for consumers on external types and provide a cleaner more maintainable package going forward, thoughts?
| ) | ||
|
|
||
| // Builder defines what is needed to build an image | ||
| type Builder interface { |
There was a problem hiding this comment.
question: does this need to be an interface or would a concrete type work better or simplify?
Also this doesn't seem to do a build, but more provide information to a builder, so is this the right name?
| GetContext() (io.Reader, error) // the path to the build context | ||
| GetDockerfile() string // the relative path to the Dockerfile, including the file itself | ||
| GetRepo() string // get repo label for image | ||
| GetTag() string // get tag label for image |
There was a problem hiding this comment.
suggestion: make these more idiomatic, removing the Get prefixes.
| @@ -0,0 +1,137 @@ | |||
| package image | |||
There was a problem hiding this comment.
suggestion: use a generated mock if we want to keep the interface.
| fileLocation := filepath.Join(targetDir, ".dockerignore") | ||
| var excluded []string | ||
| exists := false | ||
| if f, openErr := os.Open(fileLocation); openErr == nil { |
| // List list images from the provider. If an image has multiple Tags, each tag is reported | ||
| // individually with the same ID and same labels | ||
| func List(ctx context.Context) ([]Info, error) { | ||
| images := []Info{} |
There was a problem hiding this comment.
suggestion: use declared value instead
| images := []Info{} | |
| var images []Info |
| return fmt.Errorf("opening output file %w", err) | ||
| } | ||
| defer func() { | ||
| _ = outputFile.Close() |
There was a problem hiding this comment.
bug: you can loose data a not known it, name the return and assign on close.
| defer func() { | ||
| _ = imageReader.Close() | ||
| }() |
There was a problem hiding this comment.
suggestion: no need for the wrap just call Close on a reader.
| defer func() { | |
| _ = imageReader.Close() | |
| }() | |
| defer imageReader.Close() |
| _, err = outputFile.ReadFrom(imageReader) | ||
| if err != nil { |
There was a problem hiding this comment.
suggestion: fold for more idiomatic code
| _, err = outputFile.ReadFrom(imageReader) | |
| if err != nil { | |
| if _, err = outputFile.ReadFrom(imageReader); err != nil { |
| @@ -0,0 +1,66 @@ | |||
| package mock | |||
There was a problem hiding this comment.
question: is this just a move? Would a real mock be better?
|
|
||
| // Build build an image from the given Builder, using the default docker client. | ||
| // It returns the first tag of the image. | ||
| func Build(ctx context.Context, img Builder) (string, error) { |
There was a problem hiding this comment.
suggestion: having it be a Builder type and img name is confusing, lets get consistent.
2 participants
What does this PR do?
This PR refactors the code to extract the docker image related code to a top-level package:
image.This package will be responsible of:
The related tests have been moved to the package, but there are "e2e" tests (they are actually using a ContainerRequest) that have been kept in the root package of the library.
Why is it important?
Separation of concerns, reducing the API surface in the root package.