-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
postgresqlPackages.pg_net: 0.8.0 -> 0.11.0 #353532
Conversation
@NixOS/nixpkgs-merge-bot merge |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4799 |
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.
@NixOS/nixpkgs-merge-bot merge
I don't think you can reasonably expect to be able to use this command, for which you need to be a maintainer already, while adding yourself as one in the same PR. Independent of whether that's already in by-name or not.
maintainers = with maintainers; [ thoughtpolice ]; | ||
maintainers = with maintainers; [ thoughtpolice samrose ]; |
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.
Please add yourself as maintainer in a separate commit. See https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions.
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.
done in de015f4
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.
This is still in this commit and then you were just removed in a later commit.
That being said, it's kind of odd that ofborg adds the |
Also note the commit messages should start with |
Thanks for that info. It's been some time since I have put through PR into nixpkgs, and it wasn't clear from contributing docs what various permissions are needed for these actions. But that makes sense and thank you for the clarification. |
Right now you have 4 commits, but we can't merge like that. Please rebase your branch, so that you have three commits:
Please also change the PR title accordingly (with the postgreslPackages prefix). I wasn't able to find this PR when just searching for "postgresql" the first time, it didn't show up. Also some of the CI depends on the commits' prefixes, IIRC. |
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.
Except for @wolfgangwalther's remarks the change looks good to me, thanks!
This now needs a rebase to solve the expected conflicts after #344039 has reached master. |
ftr: I added the update of pg_net to #356283. |
Update pg_net to 0.11.0
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.