pass PGPASSWORD via env directly, not via shell

[evgeni]

No description provided.

[evgeni]

with this, nothing uses hidden_patterns anymore. should we drop it, so that this bad design doesn't creep up on us again?

and one more thing: technically hidden_patterns also replaces the pattern in stdout, but I would not expect Postgres to ever print the password, so 🤷‍♀️

[ehelms]

Agreed on cleaning up hidden_patterns if it's not used.

[ekohl]

Overall I think this is OK, but the whole module looks rather inconsistent.

[ekohl]

It feels like this should use psql('SHOW server_version'), but you probably also saw that and considered it out of scope.

And as always, I think it should have used --no-align instead of -A to make the comment above it redundant.

[evgeni]

maybe, but I also didn't feel like actually touching the code here

[evgeni]

I've just realized that while this fixes one issue with the passwords, it doesn't fix the one in the report. 🤦‍♀️
That is about us parsing pulp's settings.py wrongly

[ekohl]

I wonder if it could wrap django's dbshell command to run queries somehow

[evgeni]

Don't give me ideas

[evgeni]

PYTHONDONTWRITEBYTECODE=1 PYTHONPATH=/etc/pulp python -c 'import yaml; import settings; print(yaml.safe_dump(settings.DATABASES["default"]))'

[ekohl]

My biggest issue is that the Rails equivalent needs to load the entire Rails env, which is slow. And there is none (that I know of) for Candlepin. Not that we need queries on that after we merge #940, but there's still the backup/restore part.

[ekohl]

PYTHONDONTWRITEBYTECODE=1 PYTHONPATH=/etc/pulp python -c 'import yaml; import settings; print(yaml.safe_dump(settings.DATABASES["default"]))'

Nit: I'd prefer JSON and you can use pulpcore-admin shell to get a Python shell with guaranteed the correct Python version.

[evgeni]

sure, why not:

# PULP_SETTINGS=/etc/pulp/settings.py pulpcore-manager shell --command 'from django.conf import settings; import json; print(json.dumps(settings.DATABASES["default"]))'
{"ENGINE": "django.db.backends.postgresql", "NAME": "pulpcore", "USER": "pulp", "PASSWORD": "sdfsfds\"dssdfdsf", "HOST": "remotedb", "PORT": "5432", "ATOMIC_REQUESTS": false, "AUTOCOMMIT": true, "CONN_MAX_AGE": 0, "CONN_HEALTH_CHECKS": false, "OPTIONS": {}, "TIME_ZONE": null, "TEST": {"CHARSET": null, "COLLATION": null, "MIGRATE": true, "MIRROR": null, "NAME": null}}

[ekohl]

The reason I bring it up is that it's probably easier to reuse this once we start using containers. If we start using env vars or secrets, that'll work at runtime but it'll be complex to parse all the options.

[evgeni]

#941

[evgeni]

dropped the (empty) command runner tests and added a bunch of base_database tests instead

[ekohl]

I wanted to know how I'd structure it and in #945 there are the results of my refactor.

The biggest change is that instead of composing complex commands I pass all PG connection params via env vars which simplifies a lot of the code.

I can add your tests if you'd like (and adopt where needed).

[evgeni]

I can add your tests if you'd like (and adopt where needed).

I think that'd be a good idea, but maybe just merge this and then use yours as further improvement?

[ekohl]

I can add your tests if you'd like (and adopt where needed).

This is done now.

maybe just merge this and then use yours as further improvement?

I'm not sure I like refactoring it twice shortly after each other.

[evgeni]

Strictly speaking this one was never intended as refactoring, just fixing the bug we have when the password contains quotes ;)

[evgeni]

closing in favor of #945