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

Add support for re-usable containers #693

Merged
merged 7 commits into from Oct 20, 2022

Conversation

devuo
Copy link
Contributor

@devuo devuo commented Oct 18, 2022

Add support for developers to pass gnomock.WithContainerReuse() option that makes Gnomock attempt to re-use an already running container with the name specified via gnomock.WithContainerName().

If the container with the given name does not exist, or if it uses a different image than the one configured, the normal flow of Gnomock is used, namely a new container is created in case it doesn't exist, or replaced if using a different image.

Pending:

  • Agree on technical solution
  • Add inline comments
  • Write tests

@devuo
Copy link
Contributor Author

devuo commented Oct 18, 2022

Related with #691

@devuo
Copy link
Contributor Author

devuo commented Oct 18, 2022

@orlangure do you agree with the approach I followed? If yes, I'll add the comments and tests and put it to review.

@orlangure
Copy link
Owner

Very cool!

I like the idea to keep the existing behaviour unless the other option is specified, and I really like the extra bit you added with the image match: if the image changed, no need to reuse the container, it needs to be created again.

I looked at the code briefly, and all looks good. I think there might be a problem with "running" filter you used to list the containers: if there is a container with the same name that is not "running", we will try to create a new one with the same name and fail (I guess that's what would happen). Please look a bit more into that potential issue, or confirm that the behaviour will be as expected with the existing code.

@devuo
Copy link
Contributor Author

devuo commented Oct 18, 2022

@orlangure tried to reproduce the problem but was unable to. For two reasons:

  • The first is that the containers Gnomock creates have auto remove, then once they exit they are removed:
    AutoRemove: true,
  • The second because Gnomock is already handling the case of duplicate containers:

    gnomock/docker.go

    Lines 312 to 324 in 5e86a8b

    matches := duplicateContainerRegexp.FindStringSubmatch(err.Error())
    if len(matches) == 2 {
    d.log.Infow("duplicate container found, stopping", "container", matches[1])
    err = d.client.ContainerRemove(ctx, matches[1], types.ContainerRemoveOptions{
    Force: true,
    })
    if err != nil {
    return nil, fmt.Errorf("can't remove existing container: %w", err)
    }
    resp, err = d.client.ContainerCreate(ctx, containerConfig, hostConfig, nil, nil, cfg.ContainerName)
    }

@devuo
Copy link
Contributor Author

devuo commented Oct 18, 2022

@orlangure also added tests. The only test I did not add was for the "replacement scenario" (the one where the image mismatches) and that was because I didn't know which image to use other than docker.io/orlangure/gnomock-test-image.

@devuo devuo marked this pull request as ready for review October 18, 2022 15:55
@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2022

Codecov Report

Base: 85.88% // Head: 85.80% // Decreases project coverage by -0.08% ⚠️

Coverage data is based on head (5f72952) compared to base (5e86a8b).
Patch coverage: 80.48% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #693      +/-   ##
==========================================
- Coverage   85.88%   85.80%   -0.09%     
==========================================
  Files          49       49              
  Lines        2281     2310      +29     
==========================================
+ Hits         1959     1982      +23     
- Misses        167      170       +3     
- Partials      155      158       +3     
Impacted Files Coverage Δ
docker.go 89.47% <78.94%> (-0.63%) ⬇️
options.go 100.00% <100.00%> (ø)
preset/postgres/preset.go 84.21% <0.00%> (-2.64%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@devuo
Copy link
Contributor Author

devuo commented Oct 19, 2022

@orlangure fixed lint issues, should be ok now

@orlangure
Copy link
Owner

@devuo, thank you so much for working on it, and for the last sidecar touch as well 😼
I'm a bit short on time to prepare a new release, and will try to work on in ASAP, maybe tonight or during the weekend.

Copy link
Owner

@orlangure orlangure left a comment

Choose a reason for hiding this comment

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

Perfect 😼
Thank you.

@orlangure orlangure merged commit e6e2499 into orlangure:master Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants