Fix automatic provisioning of Postgres DB addons

[edmorley]

In #1363 the logic for determining when to automatically provision a Heroku Postgres database addon was adjusted to reduce false positives.

Unfortunately, this broke provisioning in all cases, since it turns out that the build system does not source the export script prior to running bin/release (like it does when running subsequent buildpacks), and so the buildpack configured env vars (such as PATH) are not set.

This meant that when bin/release used the is_module_available utility, it was calling system Python (where the Django and psycopg2 packages were not installed) and so always failing the conditional.

Unfortunately we do not currently have an easy way to test bin/release, otherwise a test case would have been added in the original PR.

I had manually tested the original PR using the Python getting started guide, but had only checked via accessing the app and trying to run the Django ./manage.py migrate command. However it seems the Django config for that project silently falls back to sqlite if DATABASE_URL is not set - so the project (and the migrate command) still worked.

For this PR I have manually tested to ensure the addon itself appears on the app.

Fix #1374.
GUS-W-11851647.

[edmorley]

I've also updated the Buildpack API docs to mention this caveat for bin/release:
https://devcenter.heroku.com/articles/buildpack-api#bin-release

[ehmatthes]

Thanks for the prompt fix, and the thorough writeup. I hope you don't mind a brief followup.

First, your description of the core issue made me feel better about not being able to find a fix myself. I had hoped to make a quick fix and file a PR instead of an issue, but it was deeper than I could dig in a reasonable time. Can you tell me where bin/release is called from? I searched this repo (code, not issues) for release, and couldn't find anywhere that it's called. Is it called from outside the buildpack?

Second, this surprised me: "Unfortunately we do not currently have an easy way to test bin/release, otherwise a test case would have been added in the original PR." I talk to a lot of Django teachers/mentors, and one of our shared frustrations about most platforms is that their demo Django project doesn't even use a database. But the python-getting-started repo does have a tiny model and a page that saves an instance of the model. Can't you write a test script that deploys this project, and tests that the db page is rendered successfully? It's an interesting test to write, when you're testing the full deployment process.

Heroku's gotten a bit of heat recently, but I'm really happy to see changes made that need to happen for long-term sustainability. Your writeup of issues like this are a great example of what corporate open source can look like.

Thanks again!

[edmorley]

Hi! No problem :-)

Can you tell me where bin/release is called from?

The Heroku build system itself calls bin/release (like it also calls bin/detect and bin/compile). Unfortunately the codebase for that isn't open source.

Longer term, we're going to migrate from the current buildpacks/build system to Cloud Native Buildpacks (https://buildpacks.io), which can more easily be run locally (using the Pack CLI), and moving much of the build system logic out of Heroku-only codebases, which will make reproducing and testing things much easier.

Can't you write a test script that deploys this project, and tests that the db page is rendered successfully?

Yes we could. The issue is that the /db route will still render fine with sqlite, which is what the getting started guide will fall back to if DATABASE_URL is not set, so the app will still appear functional. Arguably the getting started guide project should be changed so it doesn't do that, however that work is low priority and so was out of scope of the original PR here (which was requested urgently for the upcoming changes to Heroku plans). Also, there have been discussions about potentially removing the auto-addon provisioning feature entirely (in part since it existed from a pre-app.json era, plus also isn't supported by CNBs at least at the moment), so I was reluctant to spend too much time on a custom solution here.

Ideally the build system would report in the build log what the bin/release step requested, and then our buildpack integration tests could simply confirm via the build log output, which would have allowed for a much quicker/simpler test.

[ehmatthes]

Thank you so much, this is really helpful. :)