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

Support returning all IP addresses of a container #553

Merged
merged 6 commits into from
Oct 14, 2022

Conversation

gauravgahlot
Copy link
Contributor

Fixes #513

@gauravgahlot gauravgahlot requested a review from a team as a code owner October 6, 2022 04:52
Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

I've just left a few suggestions on naming. You know, naming things...

Thanks for your contribution.

docker.go Outdated Show resolved Hide resolved
docker.go Outdated Show resolved Hide resolved
docker.go Outdated Show resolved Hide resolved
docker.go Outdated Show resolved Hide resolved
@gauravgahlot
Copy link
Contributor Author

Brings it back to how I originally wrote it. Cool. 👍

@mdelapenya
Copy link
Member

BTW, could you add tests for the new method? 🙏

@gauravgahlot
Copy link
Contributor Author

BTW, could you add tests for the new method? 🙏

Should have added one implicitly. Done.

@gauravgahlot
Copy link
Contributor Author

gauravgahlot commented Oct 6, 2022

As a side note @mdelapenya, I noticed certain lint issues in the code (also verified with golangci-lint). I was thinking may be we should have a make lint target for future. If that works for you, please create an issue and assign it to me.

@mdelapenya
Copy link
Member

As a side note @mdelapenya, I noticed certain lint issues in the code (also verified with golangci-lint). I was thinking may be we should have a make lint target for future. If that works for you, please create an issue and assign it to me.

We'd like to evaluate if pre-commit works for us, therefore a check would happen at that level.

@codecov
Copy link

codecov bot commented Oct 6, 2022

Codecov Report

Merging #553 (1bcf925) into main (c0d73a6) will increase coverage by 0.07%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main     #553      +/-   ##
==========================================
+ Coverage   68.99%   69.07%   +0.07%     
==========================================
  Files          22       22              
  Lines        2174     2186      +12     
==========================================
+ Hits         1500     1510      +10     
- Misses        535      537       +2     
  Partials      139      139              
Impacted Files Coverage Δ
container.go 80.72% <ø> (ø)
docker.go 71.77% <75.00%> (+0.14%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mdelapenya mdelapenya added the feature New functionality or new behaviors on the existing one label Oct 6, 2022
@mdelapenya mdelapenya self-assigned this Oct 6, 2022
docker.go Outdated Show resolved Hide resolved
@gauravgahlot
Copy link
Contributor Author

@mdelapenya Is anything required from my end for this PR to be merged?

@mdelapenya mdelapenya merged commit 5e65c25 into testcontainers:main Oct 14, 2022
@mdelapenya
Copy link
Member

Thanks for your contribution @gauravgahlot !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or new behaviors on the existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support returning all IP addresses of a container instead of just one
2 participants