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

Don't break socket connections for postgres #540

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sur5r
Copy link

@sur5r sur5r commented Dec 19, 2024

Also, use dict.get() for default values.

Description

Fixes #490.

#521 prevents users from setting host to an empty string for socket connections. It also contradicts the Django docs referenced in #520 which clearly state that empty HOST for Postgres means local socket connections.

Checklist

Please update this checklist as you complete each item:

  • Tests have been developed for bug fixes or new functionality.
  • The changelog has been updated, if necessary.
  • Documentation has been updated, if necessary.
  • GitHub Issues closed by this PR have been linked.

By submitting this pull request I agree that all contributions comply with this project's open source license(s).

@sur5r sur5r marked this pull request as ready for review December 19, 2024 15:45
@bckohan bckohan self-requested a review December 19, 2024 16:59
@sur5r
Copy link
Author

sur5r commented Dec 19, 2024

This is odd. The test failures seem unrelated to the changes.

@Archmonger
Copy link
Contributor

It does seem to somehow be related to this PR's changes.

Just rebased #536 and got no CI errors.

Also, use dict.get() for default values.
Copy link

codecov bot commented Dec 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.44%. Comparing base (8f1307e) to head (89859ae).
Report is 16 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #540      +/-   ##
==========================================
+ Coverage   91.40%   91.44%   +0.03%     
==========================================
  Files          19       19              
  Lines         873      912      +39     
  Branches      157      154       -3     
==========================================
+ Hits          798      834      +36     
- Misses         40       43       +3     
  Partials       35       35              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sur5r
Copy link
Author

sur5r commented Dec 20, 2024

I got confused by the sqlite errors, but those seem to be expected/ok.

I couldn't track down the problem completely, but there seems to be some modification of the USER setting that breaks the interaction with get() and a default value.

At some point during the tests, self.settings.get("USER","") returns None which could only happen if None was assigned explicitely. I couldn't find that place in the code, though.

@Archmonger
Copy link
Contributor

Archmonger commented Dec 21, 2024

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.

Requiring hostname for postgresql doesn't work with socket based connections
2 participants