-
Notifications
You must be signed in to change notification settings - Fork 1
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
Remove stock JDBC client tests #278
Conversation
The integration tests in the crate repo already use the stock PostgreSQL JDBC client.
917768b
to
8b3df4f
Compare
I'd say to not remove those, as very recently helped us identify an issue: crate/crate#14754. What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi.
We converged that code into the cratedb-examples repository the other day, so I think it will be good to remove it here.
I'd say to not remove those, as very recently helped us identify an issue.
I agree. There might just be a new issue upcoming, which has been caught when upgrading org.postgresql:postgresql from 42.6.0 to 42.7.0.
With kind regards,
Andreas.
Apologies, but I don't agree to remove the tests from here, crate-qa CI is something that we monitor closely and indicates possible issues when we change things in CrateDB. Whereas |
All right. So, let's close this PR and maybe let's also add some Dependabot configurations to the QA tests here, so that errors like currently happening at GH-288 can be caught earlier in the future? Do you have any strong objections, @mfussenegger? |
Yep, let's here other's opinion on this one before closing, thx! |
The idea would've been to extend the tests in crate/crate to fill in any gaps. But probably makes sense to keep it here, given that |
Totally agree to try to move the stuff that makes sense to crate/crate repo and here only have smoke tests if necessary. |
The integration tests in the crate repo already use the stock PostgreSQL
JDBC client.