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.

Sign up for GitHub

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

Jump to bottom

getLivenessCheckPortNumbers() should return mapped port #5734

Merged
merged 27 commits into from
Sep 28, 2022
Merged

getLivenessCheckPortNumbers() should return mapped port #5734

merged 27 commits into from
Sep 28, 2022

Conversation

aidando73
Copy link
Contributor

Addresses issue #5283

kiview
kiview reviewed Aug 16, 2022
View reviewed changes
Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

While I think this fix changes the linked issue, I have a feeling that the overrides of getLivenessCheckPortNumbers() are generally not necessary anymore. However, this might require some deeper tracing of the code and create some refactoring opportunities. We just need to be aware, that getLivenessCheckPortNumbers() is part of the public API.

...es/mssqlserver/src/test/java/org/testcontainers/junit/mssqlserver/SimpleMSSQLServerTest.java Show resolved Hide resolved
@aidando73 aidando73 requested a review from a team as a code owner August 18, 2022 12:08
@aidando73 aidando73 force-pushed the get-liveness-check-mapped-port branch from 6ee7a80 to 4612773 Compare August 18, 2022 12:10
@aidando73
Copy link
Contributor Author

aidando73 commented Aug 18, 2022 •
edited
Loading

Thank you @kiview for the review - I've addressed your comments. I'm feeling more comfortable with this PR now as it gets rid of dead code and increases meaningful test coverage.

@aidando73
Copy link
Contributor Author

I've updated this with master - hopefully that fixes the build

@kiview
Copy link
Member

kiview commented Aug 19, 2022

Ah yes, sorry about this @REslim30. There was an issue with a republished Gradle plugin from which master suffered.

@kiview kiview added this to the next milestone Aug 19, 2022
@kiview
Copy link
Member

kiview commented Aug 23, 2022

Updated the PR, since CI was failing because of #5761.

@kiview kiview self-assigned this Aug 24, 2022
@aidando73
Copy link
Contributor Author

Thank you @eddumelendez for the review.

I've addressed your suggestions. Merged in master as well.

eddumelendez
eddumelendez approved these changes Sep 28, 2022
View reviewed changes
Copy link
Member

@eddumelendez eddumelendez left a comment

Choose a reason for hiding this comment

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

LGTM

@kiview kiview merged commit f54a29a into testcontainers:master Sep 28, 2022
@kiview
Copy link
Member

kiview commented Sep 28, 2022

Thanks for this nice cleanup PR @REslim30.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddumelendez eddumelendez eddumelendez approved these changes

@kiview kiview kiview approved these changes

@rnorth rnorth Awaiting requested review from rnorth

@bsideup bsideup Awaiting requested review from bsideup

Assignees

@kiview kiview

Labels
type/bug type/housekeeping
Projects
None yet
Milestone
1.17.4
Development

Successfully merging this pull request may close these issues.
3 participants
@aidando73 @kiview @eddumelendez