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: implement new MultiStrategy design #580

Merged
merged 12 commits into from Nov 21, 2022
Merged

Conversation

hhsnopek
Copy link
Contributor

@hhsnopek hhsnopek commented Oct 24, 2022

Context

This pull request implements #559, see the design for details.

TODO:

  • Add documentation
  • [ ] Refactor/Clean up Strategies

While implementing this, I struggled to extend existing strategies due to the use of Strategy and StrategyTarget abstracts. I have implemented or tested my assumptions but using Functional Options instead of Method Chaining would open the API further and expose less of the inner API details. I may suggest this in a future discussion/iteration. For this implementation, I continued the method chaining and was forced to expose Strategy#Timeout to check if the wait strategies had a timeout defined.

This pull request requires #569 to merge as the work is based on those foundational changes. See c3de1bf for the only commit used for #559

closes #559

@codecov
Copy link

codecov bot commented Oct 24, 2022

Codecov Report

Merging #580 (c3de1bf) into main (f3b2e5e) will decrease coverage by 6.65%.
The diff coverage is 11.21%.

Current head c3de1bf differs from pull request most recent head 7ddf5da. Consider uploading reports for the commit 7ddf5da to get more accurate results

@@            Coverage Diff            @@
##             main    #580      +/-   ##
=========================================
- Coverage   13.54%   6.88%   -6.66%     
=========================================
  Files          15      11       -4     
  Lines        2001     508    -1493     
=========================================
- Hits          271      35     -236     
+ Misses       1684     470    -1214     
+ Partials       46       3      -43     
Impacted Files Coverage Δ
wait/all.go 0.00% <0.00%> (ø)
wait/exec.go 0.00% <0.00%> (ø)
wait/exit.go 0.00% <0.00%> (ø)
wait/health.go 0.00% <0.00%> (ø)
wait/host_port.go 0.00% <0.00%> (ø)
wait/http.go 0.00% <0.00%> (ø)
wait/nop.go 0.00% <0.00%> (ø)
wait/sql.go 0.00% <0.00%> (ø)
wait/wait.go 100.00% <ø> (ø)
wait/log.go 59.61% <70.58%> (ø)
... and 25 more

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

@mdelapenya
Copy link
Collaborator

Do you think the branch was created from main? I see commits from the golangci-lint P. Could you please remove those commits to simplify the review 🙏 ?

@hhsnopek
Copy link
Contributor Author

@mdelapenya definitely can separate it out once before I transition it from a draft -> ready for review. I mentioned in the PR body that c3de1bf is the only commit for #559

@hhsnopek hhsnopek marked this pull request as ready for review November 8, 2022 05:36
@hhsnopek hhsnopek requested a review from a team as a code owner November 8, 2022 05:36
@hhsnopek
Copy link
Contributor Author

hhsnopek commented Nov 8, 2022

@mdelapenya I've removed the golanglint-ci commits, feel free to take another peek! I haven't had time to add documentation and clean up the existing strategies. If you're comfortable with merging as is I can follow up with Documentation & a Strategy refactor in separate pull requests

@mdelapenya
Copy link
Collaborator

@hhsnopek could you update the docs for all the wait strategies here https://golang.testcontainers.org/features/wait/introduction? 🙏

I think this is the only thing I'm missing at the moment

design: testcontainers#559

- add: Strategy#Timeout
- add: MultiStrategy#WithDeadline
- add: NopStrategy & NopStrategyTarget
- deprecate: MultiStrategy#WithStartupTimeout
@hhsnopek
Copy link
Contributor Author

@mdelapenya all set here 🚀

Copy link
Collaborator

@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.

Very well structured, thanks for the change. I'm adding a few minor comments that need to be addressed.

Once resolved, I think we can move this little one on.

As always, thanks for your hard work here!

wait/exit.go Outdated Show resolved Hide resolved
wait/all_test.go Show resolved Hide resolved
wait/exit.go Show resolved Hide resolved
wait/nop.go Show resolved Hide resolved
@mdelapenya mdelapenya self-assigned this Nov 17, 2022
@mdelapenya mdelapenya added the enhancement New feature or request label Nov 17, 2022
wait/wait.go Show resolved Hide resolved
type Strategy interface {
WaitUntilReady(context.Context, StrategyTarget) error
}

// StrategyTimeout allows MultiStrategy to configure a Strategy's Timeout
type StrategyTimeout interface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a follow-up PR, let's add docs on how to create new wait strategies, using both interfaces.

Copy link
Collaborator

@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.

Thanks for this contribution, including a superb design discussion (#559)

LGTM 👍

image

@mdelapenya mdelapenya merged commit 9ad2a50 into testcontainers:main Nov 21, 2022
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants