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: upgrade hasql-notifications to show error #3324

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

steve-chavez
Copy link
Member

@steve-chavez steve-chavez commented Mar 12, 2024

Based on diogob/hasql-notifications#21

The logs now show:

13/Mar/2024:10:52:54 -0500: Could not listen for notifications on the pgrst channel. server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. Retrying listening for notifications..

@steve-chavez steve-chavez marked this pull request as ready for review March 13, 2024 15:56
@steve-chavez steve-chavez merged commit 86e15db into PostgREST:main Mar 13, 2024
26 checks passed
Comment on lines -22 to +27
# prev.callCabal2nixWithOptions "<name>" (super.fetchFromGitHub {
# lib.dontCheck (prev.callCabal2nixWithOptions "<name>" (super.fetchFromGitHub {
# owner = "<owner>";
# repo = "<repo>";
# rev = "<commit>";
# sha256 = "<sha256>";
# }) "--subpath=<subpath>" {};
# }) "--subpath=." {});
Copy link
Member

Choose a reason for hiding this comment

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

I don't think those changes make sense for the template here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, why? I'm always lost when I see that <subpath> and lib.dontCheck is almost always the wanted behavior.

Copy link
Member

Choose a reason for hiding this comment

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

While working on compiling with GHC 9.8 inside nix on a local branch, I have added plenty of those in the last couple of weeks. My experience so far:

  • lib.dontCheck was used not very often - and even if it was, it should not be the default as in "copy & paste comes with it", but should be added once those tests actually fail.
  • Having --subpath=. hides the fact that this is configurable and not some required default setting. I had quite a few cases where I needed a subpath other than .. On the other hand, if <subpath> is in there, you might need to think about it, yes, but the solution will be obvious, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok cool. Addressed on #3326

Comment on lines +56 to +58
pkg = "hasql-notifications";
ver = "0.2.1.0";
sha256 = "sha256-MEIirDKR81KpiBOnWJbVInWevL6Kdb/XD1Qtd8e6KsQ=";
Copy link
Member

Choose a reason for hiding this comment

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

If we want to consider this as part of our bug fix, we should bump the lower bounds in postgrest.cabal to 0.2.1.0, too. This means we'd need to update stack.yaml with the extra-dep, too.

Copy link
Member

Choose a reason for hiding this comment

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

Additionally the index-state in cabal.project.freeze needs to be updated, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, those are too easy to miss. Since we have cabal and stack running on CI, shouldn't those give an error?

Copy link
Member

Choose a reason for hiding this comment

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

They won't be missed - if you take the first step before the second. Adjusting the package overlay for nix is the second step. If you need to bump dependencies, you should always go to postgrest.cabal first. Once you update the lower bound in there, everything else, including nix, will fail. But you need to update postgrest.cabal there is no way around that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants