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

feature: custom ForSQL query #451

Merged
merged 18 commits into from
Jun 28, 2022

Conversation

funvit
Copy link
Contributor

@funvit funvit commented Jun 13, 2022

  • a new setter added WithQuery

Closes #350

wait/wait.go Outdated Show resolved Hide resolved
@mdelapenya
Copy link
Collaborator

This LGTM, thanks for taking the time to contribute it! I'd miss a test demonstrating the change, but other than that, great job!

v.funtikov added 2 commits June 13, 2022 15:45
- Test failure message fixed.
@funvit
Copy link
Contributor Author

funvit commented Jun 13, 2022

Tests added.

@funvit funvit requested a review from mdelapenya June 13, 2022 12:59
@funvit
Copy link
Contributor Author

funvit commented Jun 13, 2022

So what to do with failed checks?

@mdelapenya
Copy link
Collaborator

So what to do with failed checks?

We have certain flakiness #411, but had no bandwidth to work on it yet. In any case, I re-ran your builds 🤞

@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #451 (7d1f6ff) into main (977ff3b) will increase coverage by 0.48%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main     #451      +/-   ##
==========================================
+ Coverage   68.44%   68.93%   +0.48%     
==========================================
  Files          21       21              
  Lines        1892     1896       +4     
==========================================
+ Hits         1295     1307      +12     
+ Misses        480      472       -8     
  Partials      117      117              
Impacted Files Coverage Δ
wait/sql.go 25.00% <80.00%> (+25.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 977ff3b...7d1f6ff. Read the comment docs.

@funvit
Copy link
Contributor Author

funvit commented Jun 13, 2022

So what to do with failed checks?

We have certain flakiness #411, but had no bandwidth to work on it yet. In any case, I re-ran your builds crossed_fingers

Maybe merge my #455 at first?

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.

Great contribution, thanks!

container_test.go Outdated Show resolved Hide resolved
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.

My only concern is about adding a dependency on Postgres, which is needed to tun the tests, but will be added to the project dependency on each client code running their tests with tc-go.

I understand the purpose of adding it, although I'd like to avoid adding that dependency. Do you foresee any solution for that?

@funvit
Copy link
Contributor Author

funvit commented Jun 15, 2022

My only concern is about adding a dependency on Postgres, which is needed to tun the tests, but will be added to the project dependency on each client code running their tests with tc-go.

I understand the purpose of adding it, although I'd like to avoid adding that dependency. Do you foresee any solution for that?

I can move TestContainerWithWaitForSQL to e2e directory and create there custom go.mod. This will remove dependency from main package.

After that gitlab CI file must be updated...

Makefile Outdated Show resolved Hide resolved
e2e/go.mod Show resolved Hide resolved
@mdelapenya mdelapenya requested a review from gianarb June 21, 2022 10:44
@mdelapenya
Copy link
Collaborator

mdelapenya commented Jun 28, 2022

Hey @gianarb I'd like you to take a deep look here because this PR comes with a submodule to avoid including postgres dependency as part of the project. And this is pretty related to the "canned" modules discussion we had in the past: #112

I'd appreciate your comments here

@gianarb
Copy link
Collaborator

gianarb commented Jun 28, 2022

I think in this case it is fine, there is a separate module and this is not a new feature but an enhancement for something we already support.

Makefile Outdated Show resolved Hide resolved
e2e/Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
e2e/README.md Outdated Show resolved Hide resolved
e2e/README.md Outdated Show resolved Hide resolved
funvit and others added 4 commits June 28, 2022 17:52
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@mdelapenya mdelapenya self-assigned this Jun 28, 2022
@mdelapenya mdelapenya added the feature New functionality or new behaviors on the existing one label Jun 28, 2022
@mdelapenya mdelapenya merged commit 838c6e8 into testcontainers:main Jun 28, 2022
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.

Feature request: Enable specifying query for wait.ForSQL.
3 participants