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

Patch slonik to close streams more reliably #599

Merged
merged 4 commits into from
Oct 11, 2022

Conversation

@alxndrsn alxndrsn changed the title Minimal slonik patch 23.6.2 Minimal slonik patch for closing stream queries Sep 7, 2022
@alxndrsn alxndrsn requested review from matthew-white and removed request for matthew-white September 10, 2022 15:20
Copy link
Member

@matthew-white matthew-white left a comment

Choose a reason for hiding this comment

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

I think we're hoping to ship the patched Slonik with the upcoming patch release. We'll release off the release branch so that we don't ship other commits in master. After the release, we'll merge release into master. (For what it's worth, patch releases are usually simpler than this one has been. Usually patch releases come pretty soon after the last major release, so there aren't as many differences between the release branch and master.) To add this PR to the patch release, could you target the release branch? You'll probably first need to rebase onto origin/release locally, then force-push.

package.json Outdated Show resolved Hide resolved
lib/model/query/comments.js Outdated Show resolved Hide resolved
lib/util/db.js Outdated Show resolved Hide resolved
.circleci/config.yml Show resolved Hide resolved
@alxndrsn alxndrsn changed the title Minimal slonik patch for closing stream queries Minimal slonik patch for closing stream queries (master) Sep 12, 2022
lib/util/db.js Outdated Show resolved Hide resolved
Copy link
Member

@matthew-white matthew-white left a comment

Choose a reason for hiding this comment

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

The unit tests look good to me!

lib/util/db.js Outdated Show resolved Hide resolved
lib/util/db.js Outdated Show resolved Hide resolved
@matthew-white matthew-white mentioned this pull request Sep 22, 2022
@alxndrsn alxndrsn marked this pull request as ready for review October 5, 2022 11:17
@alxndrsn
Copy link
Contributor Author

alxndrsn commented Oct 5, 2022

@matthew-white this has been rebased onto current master to re-introduce the slonik QueryStream._destroy() patch with minimal other changes.

@alxndrsn
Copy link
Contributor Author

alxndrsn commented Oct 5, 2022

Maybe worth including gajus/slonik#414?

@lognaturel
Copy link
Member

Maybe worth including gajus/slonik#414?

Unless it's really complicated for some reason I think that's a good idea.

@alxndrsn
Copy link
Contributor Author

alxndrsn commented Oct 6, 2022

gajus/slonik#414 has been incorporated

@lognaturel
Copy link
Member

lognaturel commented Oct 6, 2022

And this will fix master That change is at #635 and should probably be merged on its own.

Took a quick look, all looks like I expect, but I think it would be good for @matthew-white to take one last look since you two have all the context.

benchmarker/scripts/ci-benchmark Outdated Show resolved Hide resolved
test/integration/other/db-stream.js Outdated Show resolved Hide resolved
test/integration/other/db-stream.js Outdated Show resolved Hide resolved
@matthew-white
Copy link
Member

@alxndrsn, would you mind creating a PR to the central repo that specifies --legacy-peer-deps to npm ci? I think we'll need that in order to deploy this PR to a server.

@alxndrsn alxndrsn changed the title Minimal slonik patch for closing stream queries (master) Patch slonik to close streams more reliably Oct 7, 2022
Copy link
Member

@matthew-white matthew-white left a comment

Choose a reason for hiding this comment

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

I see that the PR description mentions that gajus/slonik#420 has been backported as well. That Slonik fix hasn't been merged yet, so I feel less sure about including that one. But it also seems OK to go ahead and merge: if we need to, we can always update our Slonik patch. Plus, we're hoping to ship #564 for the release.

I'm also unsure why import/no-extraneous-dependencies is being tripped. Do we need to configure or update eslint-plugin-import somehow? But also, there doesn't seem to be a lot of risk around these ESLint violations, so that question might not be worth digging into.

@alxndrsn
Copy link
Contributor Author

I'm also unsure why import/no-extraneous-dependencies is being tripped.

I've double-checked, and it doesn't seem to be necessary 👍

@alxndrsn
Copy link
Contributor Author

alxndrsn commented Oct 11, 2022

Won't reliably pass github-actions checks until #638 is merged

@matthew-white matthew-white merged commit 1d6404a into getodk:master Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants