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

Allow blank DB host value, reuse backend code #15736

Open
wants to merge 3 commits into
base: devel
Choose a base branch
from

Conversation

AlanCoding
Copy link
Member

SUMMARY

This avoids us create a "connection string" which was already believed to be brittle and undesirable.

Here are some research links of various value:

https://github.com/django/django/blob/main/django/db/backends/base/base.py#L30

https://github.com/django/django/blob/main/django/db/backends/postgresql/base.py

This create the same thing as django.db.connection, but using that object specifically comes with a lot of baggage. It's much cleaner to just create it, as what we need is set up in its __init__. This avoids touching the already active connection, which is being used for other stuff. The deepcopy we already had still seems like a good idea.

The idea is that "HOST" in the DATABASES setting may be an empty string. Instead of handling this specifically, I'm trying to do it the "right" way finally here. Presumably this works for password-less auth which we were already trying to allow.

Essentially, we have to "translate" the Django database settings into the psycopg parameters, and instead of doing it manually we will use the logic already written in Django.

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • API

@AlanCoding
Copy link
Member Author

Still trying to get more test results before merging, which will have to wait until the dependencies are fixed.

@AlanCoding AlanCoding marked this pull request as ready for review January 13, 2025 22:03
@AlanCoding
Copy link
Member Author

Link related associated fixes:

Reviewing these, the only valid concern that comes up is that we should also do this in awx/main/wsrelay.py. That might be important enough to block merging this until resolved. This likely dictates a utility method to convert the "settings_dict" to psycopg params. This could help with testing. We should add tests...

@AlanCoding
Copy link
Member Author

I was hesitant to include this in the current PR, so I made a new branch:

https://github.com/ansible/awx/compare/devel...AlanCoding:wsrelay_params?expand=1

Will test that separately.

@AlanCoding
Copy link
Member Author

^ that passes tests. I checked the wsrelay logs and didn't see any problems.

I have to expand the scope of this, but it's just not worth it without this broader change, so I'm pushing it. Really hate to do this, but will have to ask for a re-review with that.

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

Successfully merging this pull request may close these issues.

3 participants