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

fix(sql/testing): use wait.ForSQL when readying cockroach #1183

Merged
merged 3 commits into from
Nov 30, 2022

Conversation

GeorgeMac
Copy link
Member

I've noticed connections on cockroachdb attempting to apply migrations are getting stuck a lot during testing.

I'm unsure if this will help. However, I decided to attempt and migrate to the wait.ForSQL pattern for cockroach.
Just in case there is some kind of race when cockroach HTTP server is ready but SQL is in a bad state.

Example: https://github.com/flipt-io/flipt/actions/runs/3578054959/jobs/6017766294

@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2022

Codecov Report

Merging #1183 (6237254) into main (518ec32) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1183   +/-   ##
=======================================
  Coverage   78.83%   78.83%           
=======================================
  Files          35       35           
  Lines        2528     2528           
=======================================
  Hits         1993     1993           
  Misses        435      435           
  Partials      100      100           

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

@GeorgeMac
Copy link
Member Author

GeorgeMac commented Nov 30, 2022

Hmm didn't help.

Going to push up a change here to see the log output from cockroachdb.

@markphelps
Copy link
Collaborator

I was wondering if the cockroach image changed, like if we were using latest but it seems like we haven't updated the image being used in awhile cockroachdb/cockroach:latest-v21.2

@GeorgeMac
Copy link
Member Author

Yeah, its weird. I've literally run this about 10 times now in CI. and run it locally with -count 5 numerous times.
Can't get it to fail 😂 Tempted to merge this change as-is. Then, when it next occurs we will have context, what do you think @markphelps ?

Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

Do it to it

@GeorgeMac GeorgeMac merged commit 2638b10 into main Nov 30, 2022
@GeorgeMac GeorgeMac deleted the gm/cockroachdb-stability branch November 30, 2022 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants