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

feat(wait): skip internal host port check #2691

Merged
merged 2 commits into from
Aug 12, 2024

Conversation

stevenh
Copy link
Collaborator

@stevenh stevenh commented Aug 6, 2024

Add the ability to skip the internal host port check when performing as HostPortStrategy check. This is useful when a container does bind to the internal port until additional conditions are met.

@stevenh stevenh requested a review from a team as a code owner August 6, 2024 20:26
Copy link

netlify bot commented Aug 6, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit e5411a7
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/66b9ca74b0d8e10008687abd
😎 Deploy Preview https://deploy-preview-2691--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

func NewHostPortStrategy(port nat.Port) *HostPortStrategy {
return &HostPortStrategy{
Port: port,
PollInterval: defaultPollInterval(),
}
}

// ForExposedPortOnly returns a host port strategy that waits for the given port
// to be exposed only, it does not wait for the port to be bound inside the container.
func ForExposedPortOnly(port nat.Port) *HostPortStrategy {
Copy link
Member

Choose a reason for hiding this comment

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

I could see the value in adding a new constructor function, but I'd prefer consistency across the different wait strategies using the fluid builder pattern, something like this:

  wait.ForListeningPort().SkipInternalCheck() // or any other relevant method name

I think this would be more consistent. Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Personally I find that needlessly verbose for a common option, so opted to follow the existing example set by ForListeningPort and ForExposedPort which admittedly only configure the port.

It could also be a bit confusing because the use case for this is when the port isn't actually listening.

What do you recon?

Copy link
Member

Choose a reason for hiding this comment

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

We can probably find a better name:

  • ForExternalPort, using the comments you added for the current Only function
  • ForNotBoundPort
  • ForPort
  • ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed the Only does seem unnecessary, how about just ForExposedPort as that matches the existing docker terminology so makes it clear what its looking to achieve?

Copy link
Member

Choose a reason for hiding this comment

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

Mmm we already have the ForExposedPort that's why I'd add the fluid builder option to the strategy. In v1, we could go with functional options instead of the builder pattern for wait strategies.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep forgot I added Only for that reason and naming things is hard :)

Think I'm gonna do a 180, should we do the builder SkipInternalCheck and then review the approach for v1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Went for WithoutInternalCheck as the SkipInternalCheck conflicts with the field name.

wait/host_port.go Outdated Show resolved Hide resolved
mdelapenya
mdelapenya previously approved these changes Aug 9, 2024
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.

LGTM! will merge once the comment on private Vs public fields is addressed.

Thanks agains for your hard work

Add the ability to skip the internal host port check when performing as
HostPortStrategy check. This is useful when a container does bind to the
internal port until additional conditions are met.

Also improve wait for port function documentation while there.
@mdelapenya
Copy link
Member

Given we changed the field to be private, we can declare the new function as SkipInternalCheck. I'm doing it by myself including docs, so I will send a few commits on top of yours.

Thanks

@mdelapenya
Copy link
Member

@stevenh the CI passed, so if my changes are good to you, then we can merge, thanks!

Copy link
Collaborator Author

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@mdelapenya mdelapenya self-assigned this Aug 12, 2024
@mdelapenya mdelapenya added the feature New functionality or new behaviors on the existing one label Aug 12, 2024
@mdelapenya mdelapenya merged commit 4e9cb66 into testcontainers:main Aug 12, 2024
112 checks passed
mdelapenya added a commit that referenced this pull request Aug 16, 2024
* main:
  fix: config via environment (#2725)
  fix(redpanda): race condition on port check (#2692)
  fix: logging restart (#2697)
  fix!: docker authentication setup (#2727)
  chore: improve error wrapping (#2720)
  chore: run make tests in verbose mode (#2734)
  chore(deps): bump github.com/docker/docker from 27.1.0+incompatible to 27.1.1+incompatible (#2733)
  fix(kafka): port race on start (#2696)
  docs: fix broken doc tags (#2732)
  fix: nginx request failures (#2723)
  fix(compose): container locking (#2722)
  fix(wait): log test timeout (#2716)
  chore: increase timeout values (#2719)
  chore: remove unused parameters (#2721)
  chore(mockserver): silence warning about internal port (#2730)
  feat(wait): skip internal host port check (#2691)
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.

2 participants