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.

Sign up for GitHub

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

Jump to bottom

nixos/redis: always bind on localhost by default #162214

Merged
merged 1 commit into from
Mar 8, 2022
Merged

nixos/redis: always bind on localhost by default #162214

merged 1 commit into from
Mar 8, 2022

Conversation

ncfavier
Copy link
Member

@ncfavier ncfavier commented Feb 28, 2022 •
edited
Loading

Restore the behaviour before #142635 for named servers, essentially redoing #100195.

I believe the default value of bind was changed because #142635 made the condition for listening on TCP bind != null. But after #156004, the condition becomes port != 0 again, so bind should default to the sane localhost value.

See https://gitlab.com/simple-nixos-mailserver/nixos-mailserver/-/merge_requests/268 for even more context

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Feb 28, 2022
@ncfavier ncfavier requested review from yu-re-ka, nh2 and ju1m February 28, 2022 12:44
@ncfavier ncfavier changed the title nixos/redis: bind on localhost by default nixos/redis: always bind on localhost by default Feb 28, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Feb 28, 2022
ju1m
ju1m approved these changes Feb 28, 2022
View reviewed changes
Copy link
Contributor

@ju1m ju1m left a comment

Choose a reason for hiding this comment

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

@ncfavier thanks for spotting this, I'm appalled by what I've done, I'm only using Unix sockets so I've not taken care enough of the TCP :'(

nh2
nh2 approved these changes Feb 28, 2022
View reviewed changes
Copy link
Contributor

@nh2 nh2 left a comment

Choose a reason for hiding this comment

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

This looks correct to me.

@ncfavier
Copy link
Member Author

ncfavier commented Mar 2, 2022

@ofborg test redis

@ncfavier
Copy link
Member Author

ncfavier commented Mar 7, 2022

Can we merge this?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/452

@SuperSandro2000 SuperSandro2000 merged commit 0ffade9 into NixOS:master Mar 8, 2022
@ncfavier ncfavier deleted the redis-bind-localhost branch March 8, 2022 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ju1m ju1m ju1m approved these changes

@nh2 nh2 nh2 approved these changes

@yu-re-ka yu-re-ka Awaiting requested review from yu-re-ka

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ncfavier @nixos-discourse @nh2 @ju1m @SuperSandro2000