Skip to content

Comments

Don't break socket connections for postgres#540

Merged
Archmonger merged 2 commits intoArchmonger:masterfrom
sur5r:issue490
Jan 6, 2025
Merged

Don't break socket connections for postgres#540
Archmonger merged 2 commits intoArchmonger:masterfrom
sur5r:issue490

Conversation

@sur5r
Copy link
Contributor

@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
Contributor Author

sur5r commented Dec 19, 2024

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

@Archmonger
Copy link
Owner

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.
@codecov
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 (25a4bb7).
Report is 18 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
Contributor 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
Owner

Archmonger commented Dec 21, 2024

@Archmonger
Copy link
Owner

Will go ahead and merge this since it's a step up from current behavior. If someone in the future needs self.settings.get("USER","") then they will need to debug it within their PR.

@Archmonger Archmonger merged commit caf127e into Archmonger:master Jan 6, 2025
8 checks passed
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

3 participants